Skip to content

feat(mobile): support orchestration V2 on Android#3870

Draft
PixPMusic wants to merge 99 commits into
pingdotgg:android-dev-pr-3514from
PixPMusic:pixpmusic/mobile-orchestration-v2
Draft

feat(mobile): support orchestration V2 on Android#3870
PixPMusic wants to merge 99 commits into
pingdotgg:android-dev-pr-3514from
PixPMusic:pixpmusic/mobile-orchestration-v2

Conversation

@PixPMusic

@PixPMusic PixPMusic commented Jul 10, 2026

Copy link
Copy Markdown

Stacking

Warning

This PR is stacked on two branches: it targets #3579 (android-dev-pr-3514) and requires #2829. Because GitHub can only express one base, the branch carries a merge of t3code/codex-turn-mapping, so the diff below includes #2829's changes until #2829 lands. Review the four commits after Merge orchestration v2 (#2829).

Summary

Makes the Orchestration V2 experience work on the Android app:

  • resolve the Android/V2 merge seams: the V2 shell/thread cache rides the Android SQLite cache store, adapted to the V2 snapshot contracts
  • fix(mobile): render V2 controls on Android — Android icon mappings for every symbol used by the V2 lineage banner, activity inspector, queue controls, and work-log thread links (iOS keeps delegating to expo-symbols)
  • fix(mobile): keep stop and steer responsive — the composer send action reflects actual delivery semantics (offline/backlogged → Queue; connected active turn → Steer) and messages deliver to a busy run instead of failing; the shared control-lane and Codex acknowledgement fixes ship separately in fix(orchestrator): keep stop and steer responsive #3865
  • test(mobile): pin subagent result rendering — locks provider parity for projected subagent results in the activity views (pairs with feat(subagents): disclose projected results consistently #3866)
  • fix(mobile): avoid unsupported Hermes array sorting — Hermes 250829098.0.10 lacks Array.prototype.toSorted; release builds crashed opening a thread (ThreadRelationshipsBanner), replaced with immutable sort() equivalents plus a regression test

Validation

  • vp check, vp run typecheck — green across all 15 packages
  • focused mobile suites (composer model, outbox, activity views, relationship rows) — 32 tests passing
  • the same changes were validated on-device from the integration branch: bundled Release APK on a Pixel 7 Pro and the t3test AVD, cold launch clean with no Hermes/Nitro errors

Note

Medium Risk
Broad mobile orchestration UI and cache contract changes affect thread messaging, queueing, and reconnect behavior; risk is mitigated by focused tests but the stacked diff increases review blast radius.

Overview
Brings the mobile app onto Orchestration V2 end-to-end: shell/thread caches use shared ORCHESTRATION_CACHE_SCHEMA_VERSION and V2 snapshot shapes (projection.thread.id), and thread screens consume V2 shell projections, visibleTurnItems, and RuntimeRequestId for approvals/input instead of V1 turn/session models.

New thread UX wired to V2 workflows includes a lineage banner (fork/subagent/transfer navigation, merge-back, session detach), queue reorder/promote-to-steer, an activity inspector with structured payloads and checkpoint rollback, fork-from-assistant actions, and updated pending approval/input cards that respect live vs stale provider response capability.

Android-specific fixes: Tabler icon mappings for every SF Symbol used by those surfaces, composer Send / Steer / Queue labeling via resolveThreadComposerSendLabel and stop driven by threadRuntimeIsActive, and a Hermes crash fix replacing Array.prototype.toSorted with immutable sort() in orderThreadRelationshipRows (with regression test).

The diff also carries stacked changes (desktop userdata-v2 paths, large .plans docs, marketing copy) from the integration branch; the mobile commits above are the intended product scope.

Reviewed by Cursor Bugbot for commit 14e18da. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Add Orchestration V2 support to the Android mobile app, replacing session/turn model with runtime/run model

  • The mobile app's thread state, composer, approval/input cards, and status indicators are migrated from the v1 session/turn model to the v2 runtime/run model; useSelectedThreadDetail is replaced by useSelectedThreadProjection and useSelectedThreadVisibleTurnItems throughout.
  • New mobile UI components are added: ThreadRelationshipsBanner (fork/transfer/subagent navigation and merge-back), ThreadQueueControl (reorder and promote queued runs), ThreadActivityInspector (structured item detail view), and PendingApprovalCard/PendingUserInputCard now disable actions when responseCapability !== 'live'.
  • The server ships a full Orchestration V2 stack: ProjectionStoreV2, EventSinkV2, EventStoreV2, ProviderSessionManagerV2, OrchestratorV2, RunExecutionService, provider adapters (Codex, Claude, Cursor, Grok, OpenCode, ACP Registry), and database migrations 033–040.
  • A new Scheduled Tasks feature is introduced end-to-end: contracts, server service, SQL persistence (migration 040), settings UI (ScheduledTasksSettings), and a per-thread automations panel.
  • The MCP server gains an orchestrator toolkit (OrchestratorMcpService) exposing thread lifecycle, task delegation, and scheduling over MCP.
  • Cursor now uses @cursor/sdk directly instead of spawning an ACP runtime; CURSOR_API_KEY is required from provider environment settings.
  • Risk: Server state directory renamed from userdata to userdata-v2 and desktop app directories updated to t3code-v2; existing installations will start with a fresh state directory.
📊 Macroscope summarized a706e39. 5 files reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted

🗂️ Filtered Issues

No issues evaluated.

juliusmarminge and others added 30 commits April 17, 2026 17:29
Co-authored-by: codex <codex@users.noreply.github.com>
- Initialize provider as unchecked in a pending state
- Update initial probe message to reflect session-local status
- Type the runtime effect with `Scope`
- Build the ACP session runtime without wrapping it in `Effect.scoped`
- Use strict TurnId and ProviderItemId parsing in Codex session routing
- Decode in-memory stdio chunks in streaming mode to avoid split UTF-8 corruption
- Transfer session-owned scopes into adapter state
- Ensure runtime scopes close on stop and startup failure
- Add regression coverage for scoped lifecycle cleanup
- Close the managed native event logger when the adapter layer tears down
- Make session runtime close idempotent with an atomic closed flag
- Add coverage for flushing thread native logs on shutdown
- Use codex app-server snapshots for auth, models, and skills
- Remove legacy CLI/config discovery paths and related helpers
- Update tests for the new provider status flow
Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com>
- Document the target orchestration graph, IDs, lifecycles, and capability model
- Add Codex app-server probe fixtures and update the probe test harness
- Introduce orchestration v2 service interfaces and error types
- Add replay runtime, fixtures, and integration coverage
- Update shared contracts and probe transcripts

Co-authored-by: codex <codex@users.noreply.github.com>
- Add Codex adapter and replay harness wiring
- Introduce in-memory orchestration projections and provider registry
- Expand orchestration contracts for turn and runtime events
Co-authored-by: codex <codex@users.noreply.github.com>
- Add context transfer IDs, schemas, and projections
- Support cheap fork creation and Codex native fork rollback
- Cover fork idempotency and replay behavior in tests
- Track remaining projection, context transfer, rollback, capability, and subagent work
- Clarify current V2 baseline and debugger-only follow-ups
- Map fork and merge-back turns into stored handoffs and transfer resolutions
- Add shell snapshot projection support plus coverage tests
- Update replay fixtures and web contracts for the new turn flow
Co-authored-by: codex <codex@users.noreply.github.com>
- Move Codex replay recording into `apps/server`
- Add Claude Agent SDK replay fixtures and test harness
- Update orchestration-v2 fixture scenarios and docs
- Move Claude provider runtime logic into its own module
- Share the SDK query runner between live and replay paths
- Add replay driver error wrapping for unexpected failures
Port orchestration V2 provider adapter wiring to the provider-instance driver registry.

Co-authored-by: codex <codex@users.noreply.github.com>
- persist the selected model on run records
- surface run model selection in the debug UI
- update replay fixtures and contracts for the new field
- Record Claude SDK transcripts across multiple prompts and restart/query modes
- Add approval and tool-call replay coverage for new orchestration fixtures
- Update Claude adapter testkit to model open/prompt/permission frames
- Derive Claude SDK query options from runtime policy
- Add read-only replay fixture and policy mapping tests
- Reuse shared approval-policy fixtures across orchestrator tests

Co-authored-by: codex <codex@users.noreply.github.com>
- add active steering and interrupt-restart replay fixtures
- update Claude adapter/orchestrator turn handling for steering
- refresh replay and integration test coverage
- add interrupt and mid-tool replay fixtures for Claude and Codex
- log Claude Agent SDK protocol frames to native event traces
- project Codex commandExecution start events into orchestration updates
- Map Cursor SDK agents and runs to V2 thread and turn lifecycles
- Update MCP capability, tool, and testing guidance for SDK-based injection
juliusmarminge and others added 7 commits July 1, 2026 16:44
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Ports main's features onto the V2 orchestrator and wire protocol:

- HTTP snapshot loading (pingdotgg#3719): new OrchestrationV2ThreadDetailSnapshot
  contract, /api/orchestration/{shell,threads/:threadId} endpoints served
  from the V2 engine (orchestration-v2/http.ts), afterSequence resume on
  the V2 subscribeShell/subscribeThread WS methods, and V2-typed client
  snapshot loaders wired into shell/thread sync with warm-cache resume.
- Thread cache now persists the snapshot sequence (cache schema v3) so
  warm caches resume via afterSequence instead of refetching.
- Mobile: main's react-navigation architecture retained; branch's V2
  surfaces (relationships banner, queue control, activity inspector,
  work-log thread links, fork-from-run) ported off expo-router; thread
  status presentation and awareness diagnostics mapped to V2 runtime
  fields; pending-task start-turn helper gains creationSource "mobile".
- Retired v1-only additions from main (v1 RPC schemas, session/latestTurn
  awareness heuristics, v1 engine test updates) where the branch already
  replaced those systems with V2 equivalents.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Label the composer send action by actual delivery semantics: offline or
backlogged messages queue, and only a connected, backlog-free active
turn steers. Deliver messages to a busy run instead of failing.
Assert the thread activity and inspector views surface a projected
subagent result so provider parity regressions fail fast on mobile.
@coderabbitai

coderabbitai Bot commented Jul 10, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 28a012fa-6522-46d9-8e3c-3d1f38bb31c5

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:XXL 1,000+ changed lines (additions + deletions). labels Jul 10, 2026
@PixPMusic PixPMusic marked this pull request as draft July 10, 2026 20:29

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 14e18da. Configure here.

return "Queue";
}
return input.activeThreadBusy ? "Steer" : "Send";
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Steer label mismatches queue dispatch

Medium Severity

The composer's primary action label (Send, Queue, Steer) can misrepresent how a message will be handled. The resolveThreadComposerSendLabel function determines this label, but it can show "Steer" when the server will queue the message, and it only considers local outbox messages for the "Queue" label, ignoring server-side queued runs.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 14e18da. Configure here.

}
}
environmentShells.push(...archivedShells);
return environmentShells;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Archived shells overwrite live threads

Medium Severity

Building the relationship graph appends archived shell snapshots after active environment shells. deriveThreadRelationshipGraph keeps one node per thread id, so a stale archived row for a still-active thread replaces the live shell and can mark navigation as Archived or show wrong titles.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 14e18da. Configure here.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This graph construction (including appending archived snapshots after live shells) comes from the #2829 base via the stack merge — this PR's commits only touch the sorting calls below it. Keeping the fix out of this PR to avoid widening the diff beyond the mobile stack; it's worth raising on #2829 itself.

The composer labeled a send as Steer whenever the active turn was busy,
even when the server had runs queued for the thread — the send would
actually join that queue. Server-queued runs now count as backlog.
(selectedThread.session?.status !== "running" &&
selectedThread.session?.status !== "starting")
) {
const runtime = selectedThread?.runtime;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High threads/ThreadRouteScreen.tsx:464

handleStopThread checks selectedThread.runtime from the thread shell snapshot to decide whether to interrupt, but the busy/send state shown in the composer is derived from the projection in useThreadComposerState(). When the projection has an active run but the shell snapshot is stale, the composer shows the thread as busy while handleStopThread returns early without calling threadEnvironment.interruptTurn, making the Stop control a no-op for active runs. Consider basing the interrupt guard on the same projection-derived runtime state the composer uses.

Also found in 1 other location(s)

apps/mobile/src/features/threads/ThreadComposer.tsx:316

showStopAction now follows threadRuntimeIsActive(props.selectedThread.runtime), which treats queued as active. In a thread that only has server-queued runs, the collapsed composer switches its primary control from send/queue to the destructive stop pill, even though the expanded composer still labels the send action as Queue. This makes the collapsed composer show the wrong action and can cause users to cancel queued work instead of adding another queued message.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/mobile/src/features/threads/ThreadRouteScreen.tsx around line 464:

`handleStopThread` checks `selectedThread.runtime` from the thread shell snapshot to decide whether to interrupt, but the busy/send state shown in the composer is derived from the projection in `useThreadComposerState()`. When the projection has an active run but the shell snapshot is stale, the composer shows the thread as busy while `handleStopThread` returns early without calling `threadEnvironment.interruptTurn`, making the Stop control a no-op for active runs. Consider basing the interrupt guard on the same projection-derived runtime state the composer uses.

Also found in 1 other location(s):
- apps/mobile/src/features/threads/ThreadComposer.tsx:316 -- `showStopAction` now follows `threadRuntimeIsActive(props.selectedThread.runtime)`, which treats `queued` as active. In a thread that only has server-queued runs, the collapsed composer switches its primary control from send/queue to the destructive stop pill, even though the expanded composer still labels the send action as `Queue`. This makes the collapsed composer show the wrong action and can cause users to cancel queued work instead of adding another queued message.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

handleStopThread and its shell-snapshot guard come from the #2829 base (git blame shows only base commits on these lines); this PR's commits don't touch the stop path. Out of scope for the mobile stack — worth raising on #2829, where the same guard/composer divergence exists.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

<WorkingDurationPill startedAt={props.activeWorkStartedAt} />
) : null}

<ThreadQueueControl

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium threads/ThreadDetailScreen.tsx:453

When a thread has queued runs, the ThreadQueueControl panel rendered inside the floating overlay can overlap the ThreadFeed empty/unavailable placeholder, partially obscuring it. estimatedOverlayHeight only accounts for the composer chrome and WorkingDurationPill, but ThreadQueueControl also contributes to the overlay height. ThreadFeed receives that estimate as contentBottomInset, so it does not reserve enough space for the queue panel.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/mobile/src/features/threads/ThreadDetailScreen.tsx around line 453:

When a thread has queued runs, the `ThreadQueueControl` panel rendered inside the floating overlay can overlap the `ThreadFeed` empty/unavailable placeholder, partially obscuring it. `estimatedOverlayHeight` only accounts for the composer chrome and `WorkingDurationPill`, but `ThreadQueueControl` also contributes to the overlay height. `ThreadFeed` receives that estimate as `contentBottomInset`, so it does not reserve enough space for the queue panel.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wiring (queue-control overlay / relationships banner as topAccessory) is present verbatim in the #2829 base's ThreadDetailScreen — the diff shows it only because this stack merges t3code/codex-turn-mapping. Out of scope here; belongs on #2829.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium

import { type EnvironmentConnectionPhase } from "@t3tools/client-runtime/connection";

The ThreadRelationshipsBanner passed as topAccessory is silently dropped whenever contentPresentation.kind is "unavailable"ThreadFeed returns the placeholder early and never mounts the ListHeaderComponent that renders topAccessory, so the lineage banner and its detach/merge actions disappear when a thread is loading, disconnected, or unavailable. Consider rendering topAccessory in the placeholder path as well, or hoisting it above the early return in ThreadFeed.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/mobile/src/features/threads/ThreadDetailScreen.tsx around line 1:

The `ThreadRelationshipsBanner` passed as `topAccessory` is silently dropped whenever `contentPresentation.kind` is `"unavailable"` — `ThreadFeed` returns the placeholder early and never mounts the `ListHeaderComponent` that renders `topAccessory`, so the lineage banner and its detach/merge actions disappear when a thread is loading, disconnected, or unavailable. Consider rendering `topAccessory` in the placeholder path as well, or hoisting it above the early return in `ThreadFeed`.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wiring (queue-control overlay / relationships banner as topAccessory) is present verbatim in the #2829 base's ThreadDetailScreen — the diff shows it only because this stack merges t3code/codex-turn-mapping. Out of scope here; belongs on #2829.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm unable to act on this request because you do not have permissions within this repository.


const recording = RECORDINGS[scenario];
const workspace = await prepareWorkspace(scenario);
if (scenario === "subagent" || scenario === "tool_call_read_only" || scenario === "todo_list") {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium scripts/record-cursor-agent-sdk-replay-fixture.ts:175

writeFixtureFiles unconditionally overwrites package.json and tsconfig.json in the supplied cwd. When T3_CURSOR_REPLAY_CWD is set, prepareWorkspace returns that path with remove: false, so the script for subagent, todo_list, or tool_call_read_only overwrites the user's real project files and never cleans them up. Consider skipping writeFixtureFiles when workspace.remove is false, or writing fixture files into a subdirectory that is always cleaned up.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/scripts/record-cursor-agent-sdk-replay-fixture.ts around line 175:

`writeFixtureFiles` unconditionally overwrites `package.json` and `tsconfig.json` in the supplied `cwd`. When `T3_CURSOR_REPLAY_CWD` is set, `prepareWorkspace` returns that path with `remove: false`, so the script for `subagent`, `todo_list`, or `tool_call_read_only` overwrites the user's real project files and never cleans them up. Consider skipping `writeFixtureFiles` when `workspace.remove` is `false`, or writing fixture files into a subdirectory that is always cleaned up.

...(options.cwd === undefined ? {} : { cwd: options.cwd }),
...(options.allowedTools === undefined ? {} : { allowedTools: options.allowedTools }),
...(options.disallowedTools === undefined ? {} : { disallowedTools: options.disallowedTools }),
...(options.settings === undefined ? {} : { settings: options.settings }),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High Adapters/ClaudeAdapterV2.ts:418

loggedClaudeQueryOptions copies options.settings verbatim into the native protocol log, so credentials passed via launchArgs (e.g. --token=...) are written to the provider event log in cleartext every time a Claude query is opened. env, mcpServers, and callbacks are already reduced to presence-only indicators (hasEnvironment, hasMcpServers, hasCanUseTool) to avoid leaking secrets, but settings — which contains the free-form launchArgs string — is logged in full. Consider applying the same redaction strategy to settings as is done for env and mcpServers (e.g. logging only hasSettings: true).

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/Adapters/ClaudeAdapterV2.ts around line 418:

`loggedClaudeQueryOptions` copies `options.settings` verbatim into the native protocol log, so credentials passed via `launchArgs` (e.g. `--token=...`) are written to the provider event log in cleartext every time a Claude query is opened. `env`, `mcpServers`, and callbacks are already reduced to presence-only indicators (`hasEnvironment`, `hasMcpServers`, `hasCanUseTool`) to avoid leaking secrets, but `settings` — which contains the free-form `launchArgs` string — is logged in full. Consider applying the same redaction strategy to `settings` as is done for `env` and `mcpServers` (e.g. logging only `hasSettings: true`).

};
}

function acpMcpServers(threadId: ThreadId): ReadonlyArray<EffectAcpSchema.McpServer> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium Adapters/AcpAdapterV2.ts:282

acpMcpServers always emits type: "http" even for agents that advertise only sse MCP support, so SSE-only agents receive an incompatible server config and silently fail to connect to the MCP server — MCP tools stop working for that provider. The session's MCP endpoint transport should be derived from the agent's mcpCapabilities rather than hardcoded to http.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/Adapters/AcpAdapterV2.ts around line 282:

`acpMcpServers` always emits `type: "http"` even for agents that advertise only `sse` MCP support, so SSE-only agents receive an incompatible server config and silently fail to connect to the MCP server — MCP tools stop working for that provider. The session's MCP endpoint transport should be derived from the agent's `mcpCapabilities` rather than hardcoded to `http`.

Comment on lines +1135 to +1138
const allowedTools =
readOnlyTools !== undefined && readOnlyPolicyAllowsGlobalReads(runtimePolicy)
? readOnlyTools
: undefined;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High Adapters/ClaudeAdapterV2.ts:1135

claudeRuntimeQueryPolicyForRuntimePolicy leaves allowedTools unset for restricted read-only policies, so the tools: ["Read", "Glob", "Grep"] restriction is bypassed downstream: openQuery passes the undefined whitelist into claudeMcpQueryOverrides, which adds MCP tools without restriction. A restricted read-only sandbox can therefore still invoke mcp__* tools and escape the intended read-only limitation.

The condition on line 1136 gates allowedTools behind readOnlyPolicyAllowsGlobalReads(runtimePolicy), so only the fullAccess read-only variant gets a whitelist populated. Consider populating allowedTools for all read-only policies (or otherwise ensuring the tool restriction applies to MCP tools as well).

-  const allowedTools =
-    readOnlyTools !== undefined && readOnlyPolicyAllowsGlobalReads(runtimePolicy)
-      ? readOnlyTools
-      : undefined;
+  const allowedTools = readOnlyTools !== undefined ? readOnlyTools : undefined;
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/Adapters/ClaudeAdapterV2.ts around lines 1135-1138:

`claudeRuntimeQueryPolicyForRuntimePolicy` leaves `allowedTools` unset for restricted read-only policies, so the `tools: ["Read", "Glob", "Grep"]` restriction is bypassed downstream: `openQuery` passes the undefined whitelist into `claudeMcpQueryOverrides`, which adds MCP tools without restriction. A restricted read-only sandbox can therefore still invoke `mcp__*` tools and escape the intended read-only limitation.

The condition on line 1136 gates `allowedTools` behind `readOnlyPolicyAllowsGlobalReads(runtimePolicy)`, so only the `fullAccess` read-only variant gets a whitelist populated. Consider populating `allowedTools` for all read-only policies (or otherwise ensuring the tool restriction applies to MCP tools as well).

return undefined;
}
const args = unknownRecord(call.args) ?? {};
const fallbackCallId = `nested-${wrapperName}`;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium Adapters/CursorAdapterV2.ts:589

When two nested tool calls of the same type arrive without toolCallId, nestedToolCallFromEnvelope assigns both calls the identical fallback id nested-${wrapperName} (e.g. two readToolCalls both get nested-readToolCall). Because downstream ids are derived from callId, the second call overwrites the first, so only one tool artifact survives in the child thread timeline. Consider making the fallback id unique, e.g. by appending a counter or random suffix.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/Adapters/CursorAdapterV2.ts around line 589:

When two nested tool calls of the same type arrive without `toolCallId`, `nestedToolCallFromEnvelope` assigns both calls the identical fallback id `nested-${wrapperName}` (e.g. two `readToolCall`s both get `nested-readToolCall`). Because downstream ids are derived from `callId`, the second call overwrites the first, so only one tool artifact survives in the child thread timeline. Consider making the fallback id unique, e.g. by appending a counter or random suffix.

);
}

function permissionModeForClaudeRuntimePolicy(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High Adapters/ClaudeAdapterV2.ts:1087

permissionModeForClaudeRuntimePolicy silently widens an explicit workspaceWrite or externalSandbox sandbox policy to full access. For example, a policy with runtimeMode: "full-access", approvalPolicy: "never", and sandboxPolicy: { type: "workspaceWrite" } returns "bypassPermissions" instead of a mode that respects the sandbox restriction.

The switch on sandboxPolicyKindForClaudeRuntimePolicy falls through the workspaceWrite and externalSandbox cases without returning, so execution continues to the runtimeMode switch, which derives "bypassPermissions" from "full-access". Since the adapter relies on this mapping for enforcement and never passes the sandbox policy to the SDK, the explicit restriction is lost. Consider returning a mode that preserves the sandbox boundary for those cases, or document why the fallthrough is intentional.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/Adapters/ClaudeAdapterV2.ts around line 1087:

`permissionModeForClaudeRuntimePolicy` silently widens an explicit `workspaceWrite` or `externalSandbox` sandbox policy to full access. For example, a policy with `runtimeMode: "full-access"`, `approvalPolicy: "never"`, and `sandboxPolicy: { type: "workspaceWrite" }` returns `"bypassPermissions"` instead of a mode that respects the sandbox restriction.

The `switch` on `sandboxPolicyKindForClaudeRuntimePolicy` falls through the `workspaceWrite` and `externalSandbox` cases without returning, so execution continues to the `runtimeMode` switch, which derives `"bypassPermissions"` from `"full-access"`. Since the adapter relies on this mapping for enforcement and never passes the sandbox policy to the SDK, the explicit restriction is lost. Consider returning a mode that preserves the sandbox boundary for those cases, or document why the fallthrough is intentional.

return input.allowedTools === undefined ? {} : { allowedTools: input.allowedTools };
}
return {
allowedTools: Array.from(new Set([...(input.allowedTools ?? []), "mcp__t3-code__*"])),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium Adapters/ClaudeAdapterV2.ts:693

claudeMcpQueryOverrides always adds "mcp__t3-code__*" to allowedTools when an MCP session is present, but the pinned Claude Agent SDK version does not resolve wildcard entries against MCP tool names. This means every t3-code MCP tool call gets denied at the permission layer instead of being permitted, so the MCP-backed flow cannot execute any tool calls. The wildcard pattern needs to be expanded to concrete tool names (or handled through a different allow-list mechanism that the SDK recognizes), otherwise the MCP integration is effectively non-functional.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/Adapters/ClaudeAdapterV2.ts around line 693:

`claudeMcpQueryOverrides` always adds `"mcp__t3-code__*"` to `allowedTools` when an MCP session is present, but the pinned Claude Agent SDK version does not resolve wildcard entries against MCP tool names. This means every `t3-code` MCP tool call gets denied at the permission layer instead of being permitted, so the MCP-backed flow cannot execute any tool calls. The wildcard pattern needs to be expanded to concrete tool names (or handled through a different allow-list mechanism that the SDK recognizes), otherwise the MCP integration is effectively non-functional.

streamsReasoning: true,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium Adapters/CodexAdapterV2.ts:139

CodexProviderCapabilitiesV2.streaming.streamsReasoning is true, but the adapter never projects Codex reasoning items into any node, message, or turn_item events. The item/started and item/completed handlers cover userMessage, agentMessage, tools, plans, and web search, but not reasoning, so reasoning traces are silently dropped. Any caller that enables reasoning UI based on this capability will show empty or missing reasoning output for Codex sessions.

If streaming reasoning is not yet implemented, set streamsReasoning to false until the adapter emits reasoning items, or add handler logic that projects reasoning items into the appropriate events.

-    streamsReasoning: true,
+    streamsReasoning: false,
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.ts around line 139:

`CodexProviderCapabilitiesV2.streaming.streamsReasoning` is `true`, but the adapter never projects Codex `reasoning` items into any `node`, `message`, or `turn_item` events. The `item/started` and `item/completed` handlers cover `userMessage`, `agentMessage`, tools, plans, and web search, but not `reasoning`, so reasoning traces are silently dropped. Any caller that enables reasoning UI based on this capability will show empty or missing reasoning output for Codex sessions.

If streaming reasoning is not yet implemented, set `streamsReasoning` to `false` until the adapter emits reasoning items, or add handler logic that projects `reasoning` items into the appropriate events.


const terminalAssistantMessageId = terminalAssistantMessageIdByTurn.get(turnId);
const terminalAssistantId = terminalAssistantMessageIdByRun.get(runId);
const hiddenEntryIds = new Set(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium lib/threadActivity.ts:441

deriveThreadFeedRunFolds collapses every entry in a completed run except the terminal assistant message, so earlier assistant messages in the same run are hidden when the run is folded. Because Orchestration V2 allows multiple assistant messages under one runId (e.g., commentary plus final reply), settled runs silently drop assistant-authored content unless the user manually expands the fold. The filter at hiddenEntryIds only preserves the entry whose id matches terminalAssistantId; it does not exclude other message entries. Consider excluding all message entries with role === "assistant" from hiddenEntryIds, not just the terminal one.

Also found in 2 other location(s)

apps/mobile/src/features/threads/ThreadFeed.tsx:1529

terminalAssistantMessageIds at line 1529 is keyed only by RunId, so when a single run produces more than one assistant message, each later message overwrites the previous one. showAssistantMeta then becomes true for only the last assistant message in that run, which drops the timestamp / copy / fork controls from earlier assistant messages and, in collapsed run-fold mode, can hide earlier terminal replies from nested child threads that share the same runId.

packages/client-runtime/src/state/threadCheckpoints.ts:35

deriveThreadCheckpointSummaries derives assistantMessageId by taking projection.messages.findLast(...) for the whole run. Runs can contain multiple assistant messages, so a checkpoint from an earlier assistant turn in that run will be associated with the last assistant message instead of the message that existed when the checkpoint was captured. Review/diff UI keyed by assistantMessageId will attach the checkpoint to the wrong response.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/mobile/src/lib/threadActivity.ts around line 441:

`deriveThreadFeedRunFolds` collapses every entry in a completed run except the terminal assistant message, so earlier assistant messages in the same run are hidden when the run is folded. Because Orchestration V2 allows multiple assistant messages under one `runId` (e.g., commentary plus final reply), settled runs silently drop assistant-authored content unless the user manually expands the fold. The filter at `hiddenEntryIds` only preserves the entry whose `id` matches `terminalAssistantId`; it does not exclude other message entries. Consider excluding all `message` entries with `role === "assistant"` from `hiddenEntryIds`, not just the terminal one.

Also found in 2 other location(s):
- apps/mobile/src/features/threads/ThreadFeed.tsx:1529 -- `terminalAssistantMessageIds` at line `1529` is keyed only by `RunId`, so when a single run produces more than one assistant message, each later message overwrites the previous one. `showAssistantMeta` then becomes true for only the last assistant message in that run, which drops the timestamp / copy / fork controls from earlier assistant messages and, in collapsed run-fold mode, can hide earlier terminal replies from nested child threads that share the same `runId`.
- packages/client-runtime/src/state/threadCheckpoints.ts:35 -- `deriveThreadCheckpointSummaries` derives `assistantMessageId` by taking `projection.messages.findLast(...)` for the whole run. Runs can contain multiple assistant messages, so a checkpoint from an earlier assistant turn in that run will be associated with the last assistant message instead of the message that existed when the checkpoint was captured. Review/diff UI keyed by `assistantMessageId` will attach the checkpoint to the wrong response.

id: item.messageId,
role: item.type === "user_message" ? "user" : "assistant",
text: item.text,
attachments: item.type === "user_message" ? item.attachments : [],

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium lib/threadActivity.ts:565

buildThreadFeed hard-codes assistant message attachments to [], so assistant messages that carry attachments in the server projection are rendered without them in the mobile thread feed. The code only preserves attachments for user_message rows; the assistant_message branch should propagate item.attachments instead of discarding them.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/mobile/src/lib/threadActivity.ts around line 565:

`buildThreadFeed` hard-codes assistant message `attachments` to `[]`, so assistant messages that carry attachments in the server projection are rendered without them in the mobile thread feed. The code only preserves attachments for `user_message` rows; the `assistant_message` branch should propagate `item.attachments` instead of discarding them.

@PixPMusic

Copy link
Copy Markdown
Author

Scope note on the Macroscope sweep: because this stack merges t3code/codex-turn-mapping (see the warning at the top of the PR body), the diff — and therefore the bot review — includes all of #2829. The findings in apps/server/** and in mobile files this PR's commits don't touch (threadActivity.ts, use-selected-thread-requests.ts, the relationship-banner navigation) are #2829 base code; git blame on those lines shows only base commits. They belong on #2829, not here.

One of them is already fixed upstream in this stack: the claudeMcpQueryOverrides read-only allowlist finding is addressed by #3862 (fix(orchestrator): scope Claude MCP tool pre-approval) on the #2829 branch.

Review scope for this PR is the commits after Merge orchestration v2 (#2829).

Comment on lines +435 to +447
if (
normalized === "read" ||
normalized === "glob" ||
normalized === "grep" ||
normalized === "lsp" ||
normalized === "external_directory" ||
normalizedTool === "read" ||
normalizedTool.includes("glob") ||
normalizedTool.includes("grep") ||
normalizedTool.includes("search")
) {
return "file-read";
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium Adapters/OpenCodeAdapterV2.ts:435

openCodePermissionRequestKind returns "file-read" for any external_directory permission, but in OpenCode external_directory gates external writes, edits, patches, and bash workdir access — not just reads. When the tool name is unavailable (toolName is undefined) or doesn't match any read/edit heuristic, the request is misclassified as read-only, so a write-capable external-directory approval is presented and handled as a mere file-read request. Consider mapping external_directory to "file-change" (or "command") so the permission reflects its actual scope.

   if (
     normalized === "read" ||
     normalized === "glob" ||
     normalized === "grep" ||
     normalized === "lsp" ||
-    normalized === "external_directory" ||
     normalizedTool === "read" ||
     normalizedTool.includes("glob") ||
     normalizedTool.includes("grep") ||
     normalizedTool.includes("search")
   ) {
     return "file-read";
   }
+  if (normalized === "external_directory") {
+    return "file-change";
+  }
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/Adapters/OpenCodeAdapterV2.ts around lines 435-447:

`openCodePermissionRequestKind` returns `"file-read"` for any `external_directory` permission, but in OpenCode `external_directory` gates external writes, edits, patches, and bash workdir access — not just reads. When the tool name is unavailable (`toolName` is `undefined`) or doesn't match any read/edit heuristic, the request is misclassified as read-only, so a write-capable external-directory approval is presented and handled as a mere file-read request. Consider mapping `external_directory` to `"file-change"` (or `"command"`) so the permission reflects its actual scope.

}),
),
),
resumeThread: (threadInput) =>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High Adapters/OpenCodeAdapterV2.ts:2386

resumeThread calls only session.get and never applies threadInput.runtimePolicy, so a resumed OpenCode session keeps the permission rules it was originally created with. Since openCodePermissionRules is applied only at session.create time, resuming a thread that was created under a looser policy leaves edit/network/command permissions enabled even after the app has switched to a stricter runtimePolicy. The next turn on the resumed thread runs with stale, overly broad permissions instead of the requested policy. Consider re-applying threadInput.runtimePolicy (or re-invoking the permission-rule setup used at create time) during resume so the session's permissions match the current policy.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/Adapters/OpenCodeAdapterV2.ts around line 2386:

`resumeThread` calls only `session.get` and never applies `threadInput.runtimePolicy`, so a resumed OpenCode session keeps the permission rules it was originally created with. Since `openCodePermissionRules` is applied only at `session.create` time, resuming a thread that was created under a looser policy leaves edit/network/command permissions enabled even after the app has switched to a stricter `runtimePolicy`. The next turn on the resumed thread runs with stale, overly broad permissions instead of the requested policy. Consider re-applying `threadInput.runtimePolicy` (or re-invoking the permission-rule setup used at create time) during resume so the session's permissions match the current policy.

OrchestrationEffectExecutorV2Shape
>()("t3/orchestration-v2/EffectWorker/OrchestrationEffectExecutorV2") {}

export const executorLayer: Layer.Layer<

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium orchestration-v2/EffectWorker.ts:39

executorLayer yields ResourceCleanupService during layer construction, but ResourceCleanupService is a Context.Reference with a no-op default. The layer's required environment doesn't list it, so when the layer is built without an explicit override it captures the no-op default permanently. This means terminal.cleanup and attachment.cleanup effects silently do nothing — terminals and attachments are never cleaned up. Add ResourceCleanupService to the layer's RIn type so a real implementation must be provided, or yield it per-execution instead of at construction time.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/EffectWorker.ts around line 39:

`executorLayer` yields `ResourceCleanupService` during layer construction, but `ResourceCleanupService` is a `Context.Reference` with a no-op default. The layer's required environment doesn't list it, so when the layer is built without an explicit override it captures the no-op default permanently. This means `terminal.cleanup` and `attachment.cleanup` effects silently do nothing — terminals and attachments are never cleaned up. Add `ResourceCleanupService` to the layer's `RIn` type so a real implementation must be provided, or yield it per-execution instead of at construction time.

);
const nativeSession = unwrapData("session.get", response);
const resumedAt = yield* DateTime.now;
const providerThread = {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium Adapters/OpenCodeAdapterV2.ts:2394

The returned providerThread keeps the stale providerInstanceId from threadInput.providerThread instead of the current adapter instance's id. Only providerSessionId is overwritten, so resuming a thread created under a different OpenCode instance yields a providerThread whose providerInstanceId no longer matches the active instance. Downstream code filters provider threads by providerInstanceId, so the resumed thread cannot be matched and later turns may create a new native thread instead of continuing the conversation. Add providerInstanceId: input.providerInstanceId to the rebuilt object.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/Adapters/OpenCodeAdapterV2.ts around line 2394:

The returned `providerThread` keeps the stale `providerInstanceId` from `threadInput.providerThread` instead of the current adapter instance's id. Only `providerSessionId` is overwritten, so resuming a thread created under a different OpenCode instance yields a `providerThread` whose `providerInstanceId` no longer matches the active instance. Downstream code filters provider threads by `providerInstanceId`, so the resumed thread cannot be matched and later turns may create a new native thread instead of continuing the conversation. Add `providerInstanceId: input.providerInstanceId` to the rebuilt object.

},
});

const agent = yield* Effect.tryPromise({

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium Adapters/CursorAgentSdk.ts:331

When open is called with operation: "resume" and no agentId, the code passes undefined into Agent.resume via input.agentId!, causing an opaque SDK failure instead of a clear validation error. The non-null assertion suppresses the optional agentId without checking it. Consider validating that agentId is present before calling Agent.resume and returning a CursorAgentSdkRunnerError when it is missing.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/Adapters/CursorAgentSdk.ts around line 331:

When `open` is called with `operation: "resume"` and no `agentId`, the code passes `undefined` into `Agent.resume` via `input.agentId!`, causing an opaque SDK failure instead of a clear validation error. The non-null assertion suppresses the optional `agentId` without checking it. Consider validating that `agentId` is present before calling `Agent.resume` and returning a `CursorAgentSdkRunnerError` when it is missing.

].join("\n");
}

function makeProviderHandoffSummary(input: {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High orchestration-v2/ContextHandoffService.ts:126

makeProviderHandoffSummary discards every prior handoff turn item (if (item.type === "handoff") return []). Because prior handoff summaries are the only persisted record of already-transferred portable context (earlier provider switches, merge-back deltas), the new handoff summary silently omits that context. On a second provider switch or resume, conversation state that existed only inside previous handoff summaries is lost. Consider including prior handoff items in the summary instead of skipping them.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/ContextHandoffService.ts around line 126:

`makeProviderHandoffSummary` discards every prior `handoff` turn item (`if (item.type === "handoff") return []`). Because prior handoff summaries are the only persisted record of already-transferred portable context (earlier provider switches, merge-back deltas), the new handoff summary silently omits that context. On a second provider switch or resume, conversation state that existed only inside previous handoff summaries is lost. Consider including prior `handoff` items in the summary instead of skipping them.

Comment on lines +569 to +582
if (!requiresApproval && sandboxType === "workspaceWrite") {
const writableRoots = recordValue(sandboxPolicy, "writableRoots");
if (Array.isArray(writableRoots)) {
for (const root of writableRoots) {
if (typeof root === "string" && root.trim().length > 0) {
rules.push({
permission: "external_directory",
pattern: `${root.replace(/\/$/, "")}/*`,
action: "allow",
});
}
}
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium Adapters/OpenCodeAdapterV2.ts:569

When sandboxPolicy.type === "workspaceWrite" and requiresApproval is false, the code grants external_directory only for entries in writableRoots but never processes sandboxPolicy.readOnlyAccess. Canonical workspace-write policies carry readOnlyAccess: { type: "fullAccess" }, so OpenCode sessions created under that policy deny reads from external directories that other adapters allow. Consider adding a rule for external_directory based on readOnlyAccess (mirroring the readOnly branch) so read-only external access is granted under workspaceWrite as well.

   if (!requiresApproval && sandboxType === "workspaceWrite") {
+    const readOnlyAccess = recordValue(sandboxPolicy, "readOnlyAccess");
+    if (recordString(readOnlyAccess, "type") === "fullAccess") {
+      rules.push({ permission: "external_directory", pattern: "*", action: "allow" });
+    }
     const writableRoots = recordValue(sandboxPolicy, "writableRoots");
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/Adapters/OpenCodeAdapterV2.ts around lines 569-582:

When `sandboxPolicy.type === "workspaceWrite"` and `requiresApproval` is false, the code grants `external_directory` only for entries in `writableRoots` but never processes `sandboxPolicy.readOnlyAccess`. Canonical workspace-write policies carry `readOnlyAccess: { type: "fullAccess" }`, so OpenCode sessions created under that policy deny reads from external directories that other adapters allow. Consider adding a rule for `external_directory` based on `readOnlyAccess` (mirroring the `readOnly` branch) so read-only external access is granted under `workspaceWrite` as well.

Comment on lines +530 to +533
const selectedEffort = getModelSelectionStringOptionValue(
input.modelSelection,
"reasoningEffort",
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium Adapters/CodexAdapterV2.ts:530

buildCodexTurnStartParams ignores input.runtimePolicy.reasoningEffort and always derives effort from input.modelSelection.options. A RuntimePolicyV2 override that sets reasoningEffort is silently dropped, so turns run with the model-selected or default effort instead of the requested override. This also affects plan mode, where collaborationMode.settings.reasoning_effort falls back to "medium" instead of honoring the runtime policy. Consider reading input.runtimePolicy.reasoningEffort first and falling back to the model-selection option only when it is absent.

-    const selectedEffort = getModelSelectionStringOptionValue(
-      input.modelSelection,
-      "reasoningEffort",
-    );
+    const selectedEffort =
+      input.runtimePolicy.reasoningEffort ??
+      getModelSelectionStringOptionValue(input.modelSelection, "reasoningEffort");
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.ts around lines 530-533:

`buildCodexTurnStartParams` ignores `input.runtimePolicy.reasoningEffort` and always derives `effort` from `input.modelSelection.options`. A `RuntimePolicyV2` override that sets `reasoningEffort` is silently dropped, so turns run with the model-selected or default effort instead of the requested override. This also affects plan mode, where `collaborationMode.settings.reasoning_effort` falls back to `"medium"` instead of honoring the runtime policy. Consider reading `input.runtimePolicy.reasoningEffort` first and falling back to the model-selection option only when it is absent.

const decideMessageDispatch: CommandPolicyV2Shape["decideMessageDispatch"] = (input) => {
const activeRun = input.projection.runs.find(
(run) =>
run.status === "preparing" ||

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High orchestration-v2/CommandPolicy.ts:329

When a run has status waiting (e.g., paused for approval or user input), decideMessageDispatch selects it as activeRun and then enters the steer_active or restart_active branch. Both branches require a provider turn with status running, but a waiting run has no running turn, so the function returns a CommandPolicyMessageDispatchError ("No running providerInstanceId turn found") instead of dispatching the message. This means messages sent while a run is waiting for approval are dropped with an error rather than delivered. Consider excluding waiting from the activeRun filter, or handle waiting runs without requiring a running provider turn.

Also found in 1 other location(s)

apps/mobile/src/features/threads/ThreadQueueControl.tsx:98

ThreadQueueControl enables the Steer action whenever workflow.canPromoteToSteer is true, but deriveThreadQueueWorkflowState treats a run in waiting status as active. On V2 threads that are waiting for approval/user input, pressing this new Android button calls promoteQueuedRun, and the server's dispatchSteerIntoRun rejects it because the target run is not running and has no running provider turn. The promoted queued message therefore fails instead of being steered.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/CommandPolicy.ts around line 329:

When a run has status `waiting` (e.g., paused for approval or user input), `decideMessageDispatch` selects it as `activeRun` and then enters the `steer_active` or `restart_active` branch. Both branches require a provider turn with status `running`, but a `waiting` run has no running turn, so the function returns a `CommandPolicyMessageDispatchError` ("No running providerInstanceId turn found") instead of dispatching the message. This means messages sent while a run is waiting for approval are dropped with an error rather than delivered. Consider excluding `waiting` from the `activeRun` filter, or handle `waiting` runs without requiring a `running` provider turn.

Also found in 1 other location(s):
- apps/mobile/src/features/threads/ThreadQueueControl.tsx:98 -- `ThreadQueueControl` enables the `Steer` action whenever `workflow.canPromoteToSteer` is true, but `deriveThreadQueueWorkflowState` treats a run in `waiting` status as active. On V2 threads that are waiting for approval/user input, pressing this new Android button calls `promoteQueuedRun`, and the server's `dispatchSteerIntoRun` rejects it because the target run is not `running` and has no running provider turn. The promoted queued message therefore fails instead of being steered.

CheckpointRollbackServiceV2Shape
>()("t3/orchestration-v2/CheckpointRollbackService/CheckpointRollbackServiceV2") {}

export const layer: Layer.Layer<

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium orchestration-v2/CheckpointRollbackService.ts:46

execute is not idempotent, so retries after a partial success duplicate rollback side effects and append redundant events. If eventSink.write commits but the effect worker crashes before marking the effect complete, the at-least-once retry re-opens the provider session, calls checkpoints.restore and session.rollbackThread again, and writes another provider-thread.updated event with a fresh timestamp — even though the rollback already completed. Unlike checkpoint.capture, which guards against re-application, execute has no such check, so every retry-after-success path produces duplicate provider-thread mutations and extra domain events.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/CheckpointRollbackService.ts around line 46:

`execute` is not idempotent, so retries after a partial success duplicate rollback side effects and append redundant events. If `eventSink.write` commits but the effect worker crashes before marking the effect complete, the at-least-once retry re-opens the provider session, calls `checkpoints.restore` and `session.rollbackThread` again, and writes another `provider-thread.updated` event with a fresh timestamp — even though the rollback already completed. Unlike `checkpoint.capture`, which guards against re-application, `execute` has no such check, so every retry-after-success path produces duplicate provider-thread mutations and extra domain events.

return true;
}

const error = Cause.pretty(exit.cause);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium orchestration-v2/EffectWorker.ts:350

When the daemon shuts down and interrupts a running effect, Effect.exit at line 331 captures the interruption as Exit.Failure. The code then treats any failure as an execution error — logging it and calling outbox.retry or outbox.fail — so a normal shutdown burns retry attempts and can permanently fail an outbox entry whose provider operation never actually failed. Consider checking Cause.isInterrupted(exit.cause) (or only routing non-interruption failures through the retry/fail path) so shutdown interruptions are handled separately from genuine execution errors.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/EffectWorker.ts around line 350:

When the daemon shuts down and interrupts a running effect, `Effect.exit` at line 331 captures the interruption as `Exit.Failure`. The code then treats any failure as an execution error — logging it and calling `outbox.retry` or `outbox.fail` — so a normal shutdown burns retry attempts and can permanently fail an outbox entry whose provider operation never actually failed. Consider checking `Cause.isInterrupted(exit.cause)` (or only routing non-interruption failures through the retry/fail path) so shutdown interruptions are handled separately from genuine execution errors.

}
return projection;
}),
getThreadSnapshot: (threadId) =>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium orchestration-v2/ProjectionStore.ts:2288

getThreadSnapshot reads the projection from replayState and snapshotSequence from sequence in separate Ref accesses. An apply that fires between those reads increments sequence without being reflected in the returned projection, so callers receive a snapshot whose snapshotSequence is ahead of the projection. Clients use snapshotSequence as the resume cursor for event streaming, so the event that landed between the two reads is skipped permanently — the client gets the older projection but starts streaming after the missing event. Consider reading both values atomically (e.g., in a single Ref.modify) so snapshotSequence and the projection always describe the same point in time.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/ProjectionStore.ts around line 2288:

`getThreadSnapshot` reads the projection from `replayState` and `snapshotSequence` from `sequence` in separate `Ref` accesses. An `apply` that fires between those reads increments `sequence` without being reflected in the returned projection, so callers receive a snapshot whose `snapshotSequence` is ahead of the projection. Clients use `snapshotSequence` as the resume cursor for event streaming, so the event that landed between the two reads is skipped permanently — the client gets the older projection but starts streaming after the missing event. Consider reading both values atomically (e.g., in a single `Ref.modify`) so `snapshotSequence` and the projection always describe the same point in time.

...base,
contextHandoffs: upsertById(base.contextHandoffs, event.payload),
};
case "context-transfer.created":

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium orchestration-v2/ProjectionStore.ts:278

In the in-memory projection, context-transfer.created and context-transfer.updated only upsert the transfer into the projection for event.threadId. But the SQL-backed projection exposes each transfer on both the sourceThreadId and targetThreadId threads. So when payload.sourceThreadId !== payload.targetThreadId, getThreadProjection() returns incomplete contextTransfers for the other thread — the transfer is silently missing from one side. Consider also upserting the transfer into the projection for the other participant thread, or document the divergence from the SQL projection if it is intentional.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/ProjectionStore.ts around line 278:

In the in-memory projection, `context-transfer.created` and `context-transfer.updated` only upsert the transfer into the projection for `event.threadId`. But the SQL-backed projection exposes each transfer on both the `sourceThreadId` and `targetThreadId` threads. So when `payload.sourceThreadId !== payload.targetThreadId`, `getThreadProjection()` returns incomplete `contextTransfers` for the other thread — the transfer is silently missing from one side. Consider also upserting the transfer into the projection for the other participant thread, or document the divergence from the SQL projection if it is intentional.

const currentInstance = yield* Effect.option(getMetadata(current.instanceId));
const targetInstance = yield* Effect.option(getMetadata(targetModelSelection.instanceId));
const targetAdapter = yield* Effect.option(adapters.get(targetModelSelection.instanceId));
const currentSession = projection.providerSessions

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium orchestration-v2/ProviderSwitchService.ts:83

currentSession is selected as the newest session for the current instance without excluding terminal sessions. If a newer stopped or error session exists alongside an older live session, the plan is computed from the dead session. In a restart_and_resume transition, releaseProviderSessionIds only matches that dead session's id, so the active live session is never released and remains attached to the incompatible model. Consider filtering out stopped/error sessions before picking the newest, so the live session is the one targeted for release.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/ProviderSwitchService.ts around line 83:

`currentSession` is selected as the newest session for the current instance without excluding terminal sessions. If a newer `stopped` or `error` session exists alongside an older live session, the plan is computed from the dead session. In a `restart_and_resume` transition, `releaseProviderSessionIds` only matches that dead session's `id`, so the active live session is never released and remains attached to the incompatible model. Consider filtering out `stopped`/`error` sessions before picking the newest, so the live session is the one targeted for release.

Comment on lines +128 to +129
runId: payloadInput.runId ?? input.runId,
nodeId: payloadInput.nodeId ?? input.nodeId,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium orchestration-v2/ProviderEventIngestor.ts:128

In makeDomainEvent, payloadInput.runId ?? input.runId and payloadInput.nodeId ?? input.nodeId (lines 128–129) use nullish coalescing, so an explicit null in payloadInput is replaced with the parent value from input. This mis-tags provider artifacts that intentionally live outside a run — such as child-thread messages with runId: null — as belonging to the current root run, and downstream projection code silently leaks those artifacts into the parent run. Consider using a check that only falls back when the field is undefined, so an explicit null is preserved.

Suggested change
runId: payloadInput.runId ?? input.runId,
nodeId: payloadInput.nodeId ?? input.nodeId,
runId: payloadInput.runId !== undefined ? payloadInput.runId : input.runId,
nodeId: payloadInput.nodeId !== undefined ? payloadInput.nodeId : input.nodeId,
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/ProviderEventIngestor.ts around lines 128-129:

In `makeDomainEvent`, `payloadInput.runId ?? input.runId` and `payloadInput.nodeId ?? input.nodeId` (lines 128–129) use nullish coalescing, so an explicit `null` in `payloadInput` is replaced with the parent value from `input`. This mis-tags provider artifacts that intentionally live outside a run — such as child-thread messages with `runId: null` — as belonging to the current root run, and downstream projection code silently leaks those artifacts into the parent run. Consider using a check that only falls back when the field is `undefined`, so an explicit `null` is preserved.

session: Option.none(),
};
}
const session = yield* sessions.get(input.providerSessionId);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium orchestration-v2/ProviderTurnControlService.ts:130

load returns a ProviderTurnControlError when providerTurn.status is still "running" but the provider session runtime is already gone (sessions.get returns Option.none()). Because terminal provider events are projected asynchronously on a detached ingestion fiber, a turn can finish and have its runtime removed before the projection reflects the status change. In that window, interrupt and interruptAndAwaitTerminal fail spuriously for a turn that has already terminated, instead of treating the missing runtime as already-terminal and no-oping (or waiting for the projection to catch up). Consider returning a no-op result when the session is missing but the turn is still marked running, rather than failing immediately.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/ProviderTurnControlService.ts around line 130:

`load` returns a `ProviderTurnControlError` when `providerTurn.status` is still `"running"` but the provider session runtime is already gone (`sessions.get` returns `Option.none()`). Because terminal provider events are projected asynchronously on a detached ingestion fiber, a turn can finish and have its runtime removed before the projection reflects the status change. In that window, `interrupt` and `interruptAndAwaitTerminal` fail spuriously for a turn that has already terminated, instead of treating the missing runtime as already-terminal and no-oping (or waiting for the projection to catch up). Consider returning a no-op result when the session is missing but the turn is still marked `running`, rather than failing immediately.

});
}

for (let remaining = 1_000; remaining > 0; remaining -= 1) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium orchestration-v2/ProviderTurnControlService.ts:173

interruptAndAwaitTerminal polls the projection up to 1,000 times with setImmediate yields, then throws ProviderTurnControlError ("did not terminalize before restart") when the loop is exhausted. This is iteration-bounded, not time-bounded, so on a lightly loaded event loop the 1,000 iterations elapse in milliseconds while the detached ingestion fiber that projects terminal events is still catching up — causing the method to error out even though the turn terminalizes moments later. Consider replacing the fixed iteration count with a real clock-based timeout (e.g. Effect.timeout) so the wait duration is deterministic regardless of event-loop load.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/ProviderTurnControlService.ts around line 173:

`interruptAndAwaitTerminal` polls the projection up to 1,000 times with `setImmediate` yields, then throws `ProviderTurnControlError` ("did not terminalize before restart") when the loop is exhausted. This is iteration-bounded, not time-bounded, so on a lightly loaded event loop the 1,000 iterations elapse in milliseconds while the detached ingestion fiber that projects terminal events is still catching up — causing the method to error out even though the turn terminalizes moments later. Consider replacing the fixed iteration count with a real clock-based timeout (e.g. `Effect.timeout`) so the wait duration is deterministic regardless of event-loop load.

return true;
}
const execution = executor.execute(effect).pipe(Effect.as("executed" as const));
const cancellation = outbox

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium orchestration-v2/EffectWorker.ts:328

A cancellation that commits after the wasCancelled re-check at line 323 but before awaitCancellation(effect.id) registers its deferred is permanently lost, so the worker proceeds to execute executor.execute(effect) even though the outbox row is already cancelled. The outbox only delivers cancellation wakeups to a pre-registered deferred; there is no retry or re-check after the deferred is installed. For process-bound effects like provider-turn.start or runtime-request.respond, this means cancelled work can still be sent to the provider. Consider re-checking wasCancelled(effect.id) immediately after awaitCancellation registers its deferred, or registering the deferred before the initial wasCancelled check.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/EffectWorker.ts around line 328:

A cancellation that commits after the `wasCancelled` re-check at line 323 but before `awaitCancellation(effect.id)` registers its deferred is permanently lost, so the worker proceeds to execute `executor.execute(effect)` even though the outbox row is already `cancelled`. The outbox only delivers cancellation wakeups to a pre-registered deferred; there is no retry or re-check after the deferred is installed. For process-bound effects like `provider-turn.start` or `runtime-request.respond`, this means cancelled work can still be sent to the provider. Consider re-checking `wasCancelled(effect.id)` immediately after `awaitCancellation` registers its deferred, or registering the deferred before the initial `wasCancelled` check.

const runOrdinal = suppliedRunOrdinal ?? runRows[0]?.ordinal ?? null;
const lowerBound = runOrdinal === null ? 0 : runOrdinal * 1_000_000;
const upperBound = runOrdinal === null ? 999_999 : lowerBound + 999_999;
yield* sql`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High orchestration-v2/TurnItemPositionStore.ts:59

The allocate insert uses ON CONFLICT(thread_id, turn_item_id) DO NOTHING, which only handles duplicate turn-item ids. When two concurrent calls for different turnItemIds in the same thread/run both compute the same MAX(ordinal) + 1, the second insert hits the (thread_id, ordinal) uniqueness constraint and the effect fails with TurnItemPositionStoreError instead of retrying. This causes intermittent failures during concurrent turn-item creation. Consider retrying the allocation on a (thread_id, ordinal) conflict — e.g. loop until the SELECT … INSERT … SELECT succeeds — so that ordinal collisions resolve transparently.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/TurnItemPositionStore.ts around line 59:

The `allocate` insert uses `ON CONFLICT(thread_id, turn_item_id) DO NOTHING`, which only handles duplicate turn-item ids. When two concurrent calls for *different* `turnItemId`s in the same thread/run both compute the same `MAX(ordinal) + 1`, the second insert hits the `(thread_id, ordinal)` uniqueness constraint and the effect fails with `TurnItemPositionStoreError` instead of retrying. This causes intermittent failures during concurrent turn-item creation. Consider retrying the allocation on a `(thread_id, ordinal)` conflict — e.g. loop until the `SELECT … INSERT … SELECT` succeeds — so that ordinal collisions resolve transparently.

return {
...base,
thread:
event.payload.appThreadId === base.thread.id

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium orchestration-v2/ProjectionStore.ts:228

When a provider-thread.updated event arrives for thread A but event.payload.appThreadId refers to thread B, applyToProjection drops the activeProviderThreadId update because the appThreadId === base.thread.id check fails on thread A's projection. Thread B's projection never receives the update because applyToProjectionReplayState only applies the event to event.threadId. As a result, the owning thread's activeProviderThreadId stays stale in the in-memory replay state while the SQL store updates the correct thread row. Consider routing provider-thread.updated events to the projection identified by event.payload.appThreadId, or updating both the event's thread and the appThreadId thread's projection.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/ProjectionStore.ts around line 228:

When a `provider-thread.updated` event arrives for thread A but `event.payload.appThreadId` refers to thread B, `applyToProjection` drops the `activeProviderThreadId` update because the `appThreadId === base.thread.id` check fails on thread A's projection. Thread B's projection never receives the update because `applyToProjectionReplayState` only applies the event to `event.threadId`. As a result, the owning thread's `activeProviderThreadId` stays stale in the in-memory replay state while the SQL store updates the correct thread row. Consider routing `provider-thread.updated` events to the projection identified by `event.payload.appThreadId`, or updating both the event's thread and the `appThreadId` thread's projection.

if (usedTokens === null || usedTokens < 0) {
continue;
}

const maxTokens = asFiniteNumber(payload?.maxTokens);
const maxTokens = null;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium lib/contextWindow.ts:59

deriveLatestContextWindowSnapshot hard-codes maxTokens to null, which forces usedPercentage, remainingTokens, and remainingPercentage to all be null in every returned snapshot. ContextWindowMeter relies on these fields to render the capacity label and progress bar, so the context-window UI loses its percentage display whenever a compaction snapshot is found. Consider deriving maxTokens from the compaction payload instead of setting it to null.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/lib/contextWindow.ts around line 59:

`deriveLatestContextWindowSnapshot` hard-codes `maxTokens` to `null`, which forces `usedPercentage`, `remainingTokens`, and `remainingPercentage` to all be `null` in every returned snapshot. `ContextWindowMeter` relies on these fields to render the capacity label and progress bar, so the context-window UI loses its percentage display whenever a compaction snapshot is found. Consider deriving `maxTokens` from the compaction payload instead of setting it to `null`.

@@ -907,6 +960,25 @@ function UserTimelineRow({ row }: { row: Extract<TimelineRow, { kind: "message"
markdownCwd={ctx.markdownCwd}
/>
</div>
{(row.message.inputIntent && row.message.inputIntent !== "turn_start") ||

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium chat/MessagesTimeline.tsx:963

When a user message's projectedItem.item.status is running, the badge renders with border-destructive/25 bg-destructive/8 text-destructive and displays the text running, making an in-progress turn look like an error. The condition on lines 964-967 and 972-975 excludes only completed, pending, and waiting, so every other status — including healthy in-progress ones like running — falls into the destructive badge branch. Consider narrowing the condition to only genuine error statuses (or explicitly excluding running) so active turns aren't shown as failures.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/components/chat/MessagesTimeline.tsx around line 963:

When a user message's `projectedItem.item.status` is `running`, the badge renders with `border-destructive/25 bg-destructive/8 text-destructive` and displays the text `running`, making an in-progress turn look like an error. The condition on lines 964-967 and 972-975 excludes only `completed`, `pending`, and `waiting`, so every other status — including healthy in-progress ones like `running` — falls into the destructive badge branch. Consider narrowing the condition to only genuine error statuses (or explicitly excluding `running`) so active turns aren't shown as failures.

for (const [turnId, group] of groupsByTurnId) {
if (turnId === input.unsettledTurnId) {
for (const [runId, group] of groupsByRunId) {
if (runId === input.unsettledRunId || interruptedRunIds.has(runId)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium chat/MessagesTimeline.logic.ts:383

deriveTurnFolds excludes any run from folding as soon as it finds a persisted run_interrupt_request or run_interrupt_result event for it. Because those events are stored in the timeline, an interrupted run never collapses behind a turn-fold row again — not just for the in-session interrupt, but permanently after reload or once later turns start. The result is that interrupted conversations stay fully expanded forever instead of collapsing behind the usual "You stopped after …" summary.

The interrupt exclusion is keyed purely on persisted event presence, so it applies to every render. If only the currently active interrupt should stay expanded, gate the skip on the run being the latest unsettled one instead of on the persisted event set.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/components/chat/MessagesTimeline.logic.ts around line 383:

`deriveTurnFolds` excludes any run from folding as soon as it finds a persisted `run_interrupt_request` or `run_interrupt_result` event for it. Because those events are stored in the timeline, an interrupted run never collapses behind a turn-fold row again — not just for the in-session interrupt, but permanently after reload or once later turns start. The result is that interrupted conversations stay fully expanded forever instead of collapsing behind the usual `"You stopped after …"` summary.

The interrupt exclusion is keyed purely on persisted event presence, so it applies to every render. If only the currently active interrupt should stay expanded, gate the skip on the run being the latest unsettled one instead of on the persisted event set.

0
),
'project.created',
${project.updated_at},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium Migrations/038_ApplicationEventSource.ts:192

The project.created baseline event uses ${project.updated_at} for occurred_at instead of ${project.created_at}, so any project that was edited after creation gets a creation event timestamp equal to its last update time rather than its actual creation time. Consumers of orchestration_events.occurred_at — event history, sequencing metadata, command receipt timing, rebuild diagnostics — will see incorrect project creation times after this migration runs. The payload correctly stores createdAt, but the event column does not. Use ${project.created_at} for the occurred_at value of the project.created event.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/persistence/Migrations/038_ApplicationEventSource.ts around line 192:

The `project.created` baseline event uses `${project.updated_at}` for `occurred_at` instead of `${project.created_at}`, so any project that was edited after creation gets a creation event timestamp equal to its last update time rather than its actual creation time. Consumers of `orchestration_events.occurred_at` — event history, sequencing metadata, command receipt timing, rebuild diagnostics — will see incorrect project creation times after this migration runs. The payload correctly stores `createdAt`, but the event column does not. Use `${project.created_at}` for the `occurred_at` value of the `project.created` event.

type ScheduleMode = "fixed" | "interval";
type WorkspaceMode = "root" | "worktree" | "existing_worktree";

interface DraftState {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium settings/ScheduledTasksSettings.tsx:60

Editing a scheduled task that targets a non-default branch silently resets it to main. DraftState only stores baseRef for worktree workspaces, so when taskToDraft loads a task whose workspaceStrategy is root or existing_worktree, the branch is dropped. submit then rebuilds workspaceStrategy without it, and saving overwrites the task's branch with the default. Consider preserving the branch in DraftState for all workspace modes so round-tripping an edit does not change which code branch future runs execute against.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/components/settings/ScheduledTasksSettings.tsx around line 60:

Editing a scheduled task that targets a non-default branch silently resets it to `main`. `DraftState` only stores `baseRef` for `worktree` workspaces, so when `taskToDraft` loads a task whose `workspaceStrategy` is `root` or `existing_worktree`, the branch is dropped. `submit` then rebuilds `workspaceStrategy` without it, and saving overwrites the task's branch with the default. Consider preserving the branch in `DraftState` for all workspace modes so round-tripping an edit does not change which code branch future runs execute against.

Comment on lines +110 to +115
const questions = extractXAiAskUserQuestions(params).map((question) => ({
id: question.id,
header: question.header,
question: question.question,
options: [...question.options],
}));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium Adapters/GrokAdapterV2.ts:110

registerGrokAcpExtensions rebuilds each question object with only id, header, question, and options, dropping the multiSelect field that extractXAiAskUserQuestions sets. When Grok sends a multi-select question, the UI receives it as single-select and only one choice is returned, so Grok gets an incomplete answer array. The rebuilt object should preserve multiSelect.

Suggested change
const questions = extractXAiAskUserQuestions(params).map((question) => ({
id: question.id,
header: question.header,
question: question.question,
options: [...question.options],
}));
questions: extractXAiAskUserQuestions(params).map((question) => ({
id: question.id,
header: question.header,
question: question.question,
multiSelect: question.multiSelect,
options: [...question.options],
}))
Also found in 1 other location(s)

packages/client-runtime/src/state/threadRequests.ts:58

derivePendingThreadRequests overwrites every user_input_request question with multiSelect: false. The contracts allow UserInputQuestion.multiSelect to be true, so any pending request that legitimately accepts multiple answers will be presented as single-select only, causing the client to render the wrong UI and prevent users from submitting the intended response.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/Adapters/GrokAdapterV2.ts around lines 110-115:

`registerGrokAcpExtensions` rebuilds each question object with only `id`, `header`, `question`, and `options`, dropping the `multiSelect` field that `extractXAiAskUserQuestions` sets. When Grok sends a multi-select question, the UI receives it as single-select and only one choice is returned, so Grok gets an incomplete answer array. The rebuilt object should preserve `multiSelect`.

Also found in 1 other location(s):
- packages/client-runtime/src/state/threadRequests.ts:58 -- `derivePendingThreadRequests` overwrites every `user_input_request` question with `multiSelect: false`. The contracts allow `UserInputQuestion.multiSelect` to be `true`, so any pending request that legitimately accepts multiple answers will be presented as single-select only, causing the client to render the wrong UI and prevent users from submitting the intended response.

yield* agentAwarenessRelay.start();
}),
),
autoBootstrap: (serverConfig.autoBootstrapProjectFromCwd

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High src/serverRuntimeStartup.ts:438

A failure in resolveAutoBootstrapWelcomeTargets (e.g., from projects.bootstrap or threadLaunch.launch) now aborts the entire startup effect at line 438, causing commandGate.failCommandReady to be called so the server never accepts commands. Previously this optional auto-bootstrap step was forked and its failures were caught and logged as a warning, so the server remained usable even when the workspace/project bootstrap failed. Consider wrapping the autoBootstrap phase in an error handler (or reverting to a forked-and-logged pattern) so bootstrap failures degrade gracefully without blocking server command readiness.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/serverRuntimeStartup.ts around line 438:

A failure in `resolveAutoBootstrapWelcomeTargets` (e.g., from `projects.bootstrap` or `threadLaunch.launch`) now aborts the entire `startup` effect at line 438, causing `commandGate.failCommandReady` to be called so the server never accepts commands. Previously this optional auto-bootstrap step was forked and its failures were caught and logged as a warning, so the server remained usable even when the workspace/project bootstrap failed. Consider wrapping the `autoBootstrap` phase in an error handler (or reverting to a forked-and-logged pattern) so bootstrap failures degrade gracefully without blocking server command readiness.

Comment on lines +173 to +178
const resolveFavicon = Effect.fn("ProjectEnrichmentService.resolveFavicon")(function* (
workspaceRoot: string,
) {
const faviconPath = yield* Cache.get(faviconCache, workspaceRoot);
yield* logFailure(workspaceRoot, "faviconPath", faviconPath);
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium project/ProjectEnrichmentService.ts:173

resolveFavicon never publishes to changes, so when a workspace's favicon resolves after its repository identity has already emitted, subscribers to subscribeChanges never receive the favicon update and remain stuck with faviconPath: null. The repository-identity lane publishes a change event that includes the current favicon value, but the favicon lane only logs failures without emitting its own event on completion. Consider publishing a ProjectEnrichmentChange from resolveFavicon after the cache resolves, mirroring what resolveRepositoryIdentity does.

  const resolveFavicon = Effect.fn("ProjectEnrichmentService.resolveFavicon")(function* (
    workspaceRoot: string,
  ) {
    const faviconPath = yield* Cache.get(faviconCache, workspaceRoot);
    yield* logFailure(workspaceRoot, "faviconPath", faviconPath);
+    const repositoryIdentity = yield* Cache.getSuccess(repositoryIdentityCache, workspaceRoot);
+    yield* PubSub.publish(changes, {
+      workspaceRoot,
+      repositoryIdentityResolved: Exit.isSuccess(repositoryIdentity),
+      enrichment: {
+        repositoryIdentity: availableValue(repositoryIdentity),
+        faviconPath: availableValue(faviconPath),
+      },
+    });
  });
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/project/ProjectEnrichmentService.ts around lines 173-178:

`resolveFavicon` never publishes to `changes`, so when a workspace's favicon resolves after its repository identity has already emitted, subscribers to `subscribeChanges` never receive the favicon update and remain stuck with `faviconPath: null`. The repository-identity lane publishes a change event that includes the current favicon value, but the favicon lane only logs failures without emitting its own event on completion. Consider publishing a `ProjectEnrichmentChange` from `resolveFavicon` after the cache resolves, mirroring what `resolveRepositoryIdentity` does.

updated_at TEXT NOT NULL
)
`;
yield* sql`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium Migrations/035_OrchestrationV2Foundation.ts:158

The orchestration_v2_projection_metadata seed row is inserted with schema_version = 1, but the runtime's ProjectionMaintenanceV2.verify expects ORCHESTRATION_V2_PROJECTION_SCHEMA_VERSION = 2. Because verify compares the stored version to that constant, every freshly migrated database is flagged as invalid and forced through a full V2 projection rebuild on the next boot. Consider seeding with 2 so the version matches what the runtime expects.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/persistence/Migrations/035_OrchestrationV2Foundation.ts around line 158:

The `orchestration_v2_projection_metadata` seed row is inserted with `schema_version = 1`, but the runtime's `ProjectionMaintenanceV2.verify` expects `ORCHESTRATION_V2_PROJECTION_SCHEMA_VERSION = 2`. Because `verify` compares the stored version to that constant, every freshly migrated database is flagged as invalid and forced through a full V2 projection rebuild on the next boot. Consider seeding with `2` so the version matches what the runtime expects.

<span className="text-destructive">-{item.deletions ?? 0}</span>
</span>
) : null}
{item.runId !== null ? (

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium chat/V2ItemInspector.tsx:160

The "Open diff" button in the file_change inspector calls props.onOpenTurnDiff(item.runId!, item.fileName) using the active thread's context, so for inherited file-change rows it opens the wrong thread's diff (or no diff at all) instead of the source thread where the run actually happened. The onOpenTurnDiff implementation resolves the diff against the active thread, not the projectedItem.sourceThreadId where item.runId originates. Consider threading props.projectedItem.sourceThreadId through onOpenTurnDiff so it opens the correct run's diff.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/components/chat/V2ItemInspector.tsx around line 160:

The "Open diff" button in the `file_change` inspector calls `props.onOpenTurnDiff(item.runId!, item.fileName)` using the active thread's context, so for inherited file-change rows it opens the wrong thread's diff (or no diff at all) instead of the source thread where the run actually happened. The `onOpenTurnDiff` implementation resolves the diff against the active thread, not the `projectedItem.sourceThreadId` where `item.runId` originates. Consider threading `props.projectedItem.sourceThreadId` through `onOpenTurnDiff` so it opens the correct run's diff.

// persistently and get tombstoned instead of published as completed.
if (thread.latestTurn?.state === "interrupted" && thread.latestTurn.completedAt !== null) {
return "completed";
if (thread.pendingRuntimeRequest !== null) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium src/agentAwareness.ts:90

resolveThreadAwarenessPhaseV2 returns "waiting_for_approval" for any non-null pendingRuntimeRequest that isn't user_input, which includes auth_refresh. Threads waiting on an auth refresh are published as actionable approval-needed rows even though there is no approval for the user to give. Consider excluding auth_refresh from the waiting_for_approval branch, matching the rest of the client model.

🤖 Copy this AI Prompt to have your agent fix this:
In file @packages/shared/src/agentAwareness.ts around line 90:

`resolveThreadAwarenessPhaseV2` returns `"waiting_for_approval"` for any non-null `pendingRuntimeRequest` that isn't `user_input`, which includes `auth_refresh`. Threads waiting on an auth refresh are published as actionable approval-needed rows even though there is no approval for the user to give. Consider excluding `auth_refresh` from the `waiting_for_approval` branch, matching the rest of the client model.

Comment on lines +146 to +149
requestedAt: null,
startedAt: null,
completedAt:
thread.status === "idle" || terminalRunStatus(thread.status) ? updatedAt : null,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium state/models.ts:146

presentThreadShell always sets latestRun.startedAt to null for V2 shell threads that have a latestRunId, so downstream consumers that treat a run as settled only when both startedAt and completedAt are present never see it as settled. For example, the Android "Plan Ready" badge gates on isLatestRunSettled(thread), which requires startedAt, so plan-mode threads loaded from a shell snapshot never show Plan Ready even after the run finishes and hasActionableProposedPlan is true. Consider populating startedAt (e.g. from thread.updatedAt or another available timestamp) so shell-derived runs are recognized as settled by those consumers.

Suggested change
requestedAt: null,
startedAt: null,
completedAt:
thread.status === "idle" || terminalRunStatus(thread.status) ? updatedAt : null,
requestedAt: null,
startedAt: updatedAt,
completedAt:
🤖 Copy this AI Prompt to have your agent fix this:
In file @packages/client-runtime/src/state/models.ts around lines 146-149:

`presentThreadShell` always sets `latestRun.startedAt` to `null` for V2 shell threads that have a `latestRunId`, so downstream consumers that treat a run as settled only when both `startedAt` and `completedAt` are present never see it as settled. For example, the Android "Plan Ready" badge gates on `isLatestRunSettled(thread)`, which requires `startedAt`, so plan-mode threads loaded from a shell snapshot never show `Plan Ready` even after the run finishes and `hasActionableProposedPlan` is `true`. Consider populating `startedAt` (e.g. from `thread.updatedAt` or another available timestamp) so shell-derived runs are recognized as settled by those consumers.

return projection.checkpoints.flatMap((checkpoint) => {
if (checkpoint.appRunOrdinal === null || checkpoint.runId === null) return [];
const assistantMessageId =
projection.messages.findLast(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium state/threadCheckpoints.ts:35

deriveThreadCheckpointSummaries derives assistantMessageId by calling projection.messages.findLast across the entire run. When a run contains multiple assistant messages, every checkpoint in that run is associated with the last assistant message rather than the one that existed when the checkpoint was captured. Any review/diff UI keyed on assistantMessageId will attach checkpoints from earlier turns to the wrong response. Consider matching on a per-turn criterion (e.g. message ordinal relative to checkpoint.appRunOrdinal) instead of findLast over the whole run.

🤖 Copy this AI Prompt to have your agent fix this:
In file @packages/client-runtime/src/state/threadCheckpoints.ts around line 35:

`deriveThreadCheckpointSummaries` derives `assistantMessageId` by calling `projection.messages.findLast` across the entire run. When a run contains multiple assistant messages, every checkpoint in that run is associated with the **last** assistant message rather than the one that existed when the checkpoint was captured. Any review/diff UI keyed on `assistantMessageId` will attach checkpoints from earlier turns to the wrong response. Consider matching on a per-turn criterion (e.g. message ordinal relative to `checkpoint.appRunOrdinal`) instead of `findLast` over the whole run.

Comment on lines +447 to +448
},
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High operations/commands.ts:447

startThreadTurn drops input.bootstrap?.runSetupScript when launching via ORCHESTRATION_V2_WS_METHODS.launchThread, so setup scripts never run on the initial launch path even though callers set runSetupScript: true. The launch request object built on line 432 omits the field entirely, causing the newly launched thread/worktree to skip the expected project bootstrap. Consider including runSetupScript in the launch payload when input.bootstrap provides it.

      initialMessage: {
        messageId: input.message.messageId,
        text: input.message.text,
        attachments,
      },
+      ...(input.bootstrap?.runSetupScript === undefined
+        ? {}
+        : { runSetupScript: input.bootstrap.runSetupScript }),
    });
🤖 Copy this AI Prompt to have your agent fix this:
In file @packages/client-runtime/src/operations/commands.ts around lines 447-448:

`startThreadTurn` drops `input.bootstrap?.runSetupScript` when launching via `ORCHESTRATION_V2_WS_METHODS.launchThread`, so setup scripts never run on the initial launch path even though callers set `runSetupScript: true`. The launch request object built on line 432 omits the field entirely, causing the newly launched thread/worktree to skip the expected project bootstrap. Consider including `runSetupScript` in the launch payload when `input.bootstrap` provides it.

Comment on lines +156 to +159
concurrency: {
mode: "serial",
key: ({ environmentId, input }) => JSON.stringify([environmentId, input.sourceThreadId]),
},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium state/threadCommands.ts:156

forkFromRun serializes only on [environmentId, input.sourceThreadId], so a command targeting the forked thread (e.g. startTurn on targetThreadId) is not serialized behind the fork. If the follow-up command reaches the server before the fork creates the target thread and its pending transfer, it starts a normal run without fork context or fails to load. The serial key should include targetThreadId (or the fork should share a lane with subsequent commands on that thread) so follow-up commands wait for the fork to complete.

Suggested change
concurrency: {
mode: "serial",
key: ({ environmentId, input }) => JSON.stringify([environmentId, input.sourceThreadId]),
},
concurrency: {
mode: "serial",
key: ({ environmentId, input }) => JSON.stringify([environmentId, input.sourceThreadId, input.targetThreadId]),
}
🤖 Copy this AI Prompt to have your agent fix this:
In file @packages/client-runtime/src/state/threadCommands.ts around lines 156-159:

`forkFromRun` serializes only on `[environmentId, input.sourceThreadId]`, so a command targeting the forked thread (e.g. `startTurn` on `targetThreadId`) is **not** serialized behind the fork. If the follow-up command reaches the server before the fork creates the target thread and its pending transfer, it starts a normal run without fork context or fails to load. The serial key should include `targetThreadId` (or the fork should share a lane with subsequent commands on that thread) so follow-up commands wait for the fork to complete.

: projection.thread.lineage.parentThreadId;
}

function edgeKey(edge: ThreadRelationshipEdge): string {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium state/threadRelationships.ts:43

deriveThreadRelationshipGraph deduplicates edges by sourceThreadId/targetThreadId/kind, so when multiple contextTransfers exist between the same two threads, all but the last are silently overwritten in edgesByKey and dropped from the returned edges array. Distinct transfers can legitimately coexist (each has its own id and statuses like superseded/consumed), so callers lose the ability to render or inspect those relationships. Consider including a discriminator (e.g., the transfer id) in edgeKey for transfer edges, or switching to an append-only structure for transfers.

🤖 Copy this AI Prompt to have your agent fix this:
In file @packages/client-runtime/src/state/threadRelationships.ts around line 43:

`deriveThreadRelationshipGraph` deduplicates edges by `sourceThreadId`/`targetThreadId`/`kind`, so when multiple `contextTransfers` exist between the same two threads, all but the last are silently overwritten in `edgesByKey` and dropped from the returned `edges` array. Distinct transfers can legitimately coexist (each has its own `id` and statuses like `superseded`/`consumed`), so callers lose the ability to render or inspect those relationships. Consider including a discriminator (e.g., the transfer `id`) in `edgeKey` for transfer edges, or switching to an append-only structure for transfers.

Comment on lines +125 to +134
if (relationshipRows.length === 0) {
return null;
}

const openThread = (threadId: ThreadId) => {
void navigate({
to: "/$environmentId/$threadId",
params: buildThreadRouteParams(scopeThreadRef(props.environmentId, threadId)),
});
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium chat/ThreadRelationshipsControl.tsx:125

ThreadRelationshipsPanel returns null when relationshipRows.length === 0, which hides the entire section — including the "Disconnect agent session" menu — even on detachable threads. canDetach depends only on an active provider session, not on relationship rows, so a thread with no lineage relationships but an active session silently loses access to the detach action. Consider rendering the section header (with the detach menu) whenever canDetach is true, not only when there are relationship rows.

-  if (relationshipRows.length === 0) {
-    return null;
-  }
-
-  const openThread = (threadId: ThreadId) => {
+  const openThread = (threadId: ThreadId) => {
+  if (relationshipRows.length === 0 && !canDetach) {
+    return null;
+  }
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/components/chat/ThreadRelationshipsControl.tsx around lines 125-134:

`ThreadRelationshipsPanel` returns `null` when `relationshipRows.length === 0`, which hides the entire section — including the "Disconnect agent session" menu — even on detachable threads. `canDetach` depends only on an active provider session, not on relationship rows, so a thread with no lineage relationships but an active session silently loses access to the detach action. Consider rendering the section header (with the detach menu) whenever `canDetach` is true, not only when there are relationship rows.

() => [] as const,
),
...(discoveryWarning ? { discoveryWarning } : {}),
parsed: {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium Layers/CursorProvider.ts:334

When the SDK catalog read succeeds, checkCursorProviderStatus hard-codes version: null in the parsed probe data, so every healthy Cursor snapshot loses its version and update-advisory information. buildServerProvider() only produces versionAdvisory metadata when a real version string is passed, so users will never see Cursor version/update status even on successful provider checks. Consider populating version from snapshot (or another available source) on the success path instead of hard-coding null.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Layers/CursorProvider.ts around line 334:

When the SDK catalog read succeeds, `checkCursorProviderStatus` hard-codes `version: null` in the `parsed` probe data, so every healthy Cursor snapshot loses its version and update-advisory information. `buildServerProvider()` only produces `versionAdvisory` metadata when a real version string is passed, so users will never see Cursor version/update status even on successful provider checks. Consider populating `version` from `snapshot` (or another available source) on the success path instead of hard-coding `null`.

Comment on lines +36 to +41
const attachedProviderThread =
activeProviderThread ??
projection.providerThreads.find(
(thread) => thread.appThreadId === projection.thread.id && thread.providerSessionId !== null,
) ??
null;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium state/threadWorkflows.ts:36

resolveThreadProviderSession falls back to the first provider thread matching the app thread id, not the most recent one. When activeProviderThreadId is null, it can bind to an old, stopped/errored session instead of the latest attached session, causing canDetachThreadProviderSession to wrongly return false and hiding supported queue/steer actions. Consider using findLast (and sorting by recency) so the newest attached provider thread wins.

  const attachedProviderThread =
    activeProviderThread ??
-    projection.providerThreads.find(
-      (thread) => thread.appThreadId === projection.thread.id && thread.providerSessionId !== null,
-    ) ??
-    null;
+    projection.providerThreads.findLast(
+      (thread) => thread.appThreadId === projection.thread.id && thread.providerSessionId !== null,
+    ) ??
+    null;
🤖 Copy this AI Prompt to have your agent fix this:
In file @packages/client-runtime/src/state/threadWorkflows.ts around lines 36-41:

`resolveThreadProviderSession` falls back to the first provider thread matching the app thread id, not the most recent one. When `activeProviderThreadId` is null, it can bind to an old, stopped/errored session instead of the latest attached session, causing `canDetachThreadProviderSession` to wrongly return `false` and hiding supported queue/steer actions. Consider using `findLast` (and sorting by recency) so the newest attached provider thread wins.

return [...ids];
}

export function walkThreadRelationships(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium state/threadRelationships.ts:119

walkThreadRelationships skips every edge to a thread after the first one that reaches it, so multiple relationships between the same two threads are silently dropped. For example, if a child is connected by both a subagent edge and a transfer edge, only whichever edge appears first in graph.edges is emitted and the other disappears from the result. Since each ThreadRelationshipWalkRow carries a single edge, callers see only one relationship where two exist.

The visited set is keyed by ThreadId alone, so once a thread is enqueued all remaining edges to it are skipped. Consider keying visited by edge (e.g. edgeKey) instead of by thread id, so the walk emits all distinct edges while still avoiding traversal cycles.

🤖 Copy this AI Prompt to have your agent fix this:
In file @packages/client-runtime/src/state/threadRelationships.ts around line 119:

`walkThreadRelationships` skips every edge to a thread after the first one that reaches it, so multiple relationships between the same two threads are silently dropped. For example, if a child is connected by both a `subagent` edge and a `transfer` edge, only whichever edge appears first in `graph.edges` is emitted and the other disappears from the result. Since each `ThreadRelationshipWalkRow` carries a single `edge`, callers see only one relationship where two exist.

The `visited` set is keyed by `ThreadId` alone, so once a thread is enqueued all remaining edges to it are skipped. Consider keying `visited` by edge (e.g. `edgeKey`) instead of by thread id, so the walk emits all distinct edges while still avoiding traversal cycles.

);
const nativeSession = unwrapData("session.get", response);
const resumedAt = yield* DateTime.now;
const providerThread = {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High Adapters/OpenCodeAdapterV2.ts:2394

The returned providerThread keeps the stale providerInstanceId from threadInput.providerThread instead of the current adapter instance's id. Only providerSessionId is overwritten, so resuming a thread created under a different OpenCode instance yields a providerThread whose providerInstanceId no longer matches the active instance. Downstream code filters provider threads by providerInstanceId, so the resumed thread cannot be matched and later turns may create a new native thread instead of continuing the conversation. Add providerInstanceId: input.providerInstanceId to the rebuilt object.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/Adapters/OpenCodeAdapterV2.ts around line 2394:

The returned `providerThread` keeps the stale `providerInstanceId` from `threadInput.providerThread` instead of the current adapter instance's id. Only `providerSessionId` is overwritten, so resuming a thread created under a different OpenCode instance yields a `providerThread` whose `providerInstanceId` no longer matches the active instance. Downstream code filters provider threads by `providerInstanceId`, so the resumed thread cannot be matched and later turns may create a new native thread instead of continuing the conversation. Add `providerInstanceId: input.providerInstanceId` to the rebuilt object.

}),
),
),
resumeThread: (threadInput) =>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High Adapters/OpenCodeAdapterV2.ts:2386

resumeThread calls only session.get and never applies threadInput.runtimePolicy, so a resumed OpenCode session keeps the permission rules it was originally created with. Since openCodePermissionRules is applied only at session.create time, resuming a thread that was created under a looser policy leaves edit/network/command permissions enabled even after the app has switched to a stricter runtimePolicy. The next turn on the resumed thread runs with stale, overly broad permissions instead of the requested policy. Consider re-applying threadInput.runtimePolicy (or re-invoking the permission-rule setup used at create time) during resume so the session's permissions match the current policy.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/Adapters/OpenCodeAdapterV2.ts around line 2386:

`resumeThread` calls only `session.get` and never applies `threadInput.runtimePolicy`, so a resumed OpenCode session keeps the permission rules it was originally created with. Since `openCodePermissionRules` is applied only at `session.create` time, resuming a thread that was created under a looser policy leaves edit/network/command permissions enabled even after the app has switched to a stricter `runtimePolicy`. The next turn on the resumed thread runs with stale, overly broad permissions instead of the requested policy. Consider re-applying `threadInput.runtimePolicy` (or re-invoking the permission-rule setup used at create time) during resume so the session's permissions match the current policy.

? null
: (projection.providerThreads.find((candidate) => candidate.id === item.providerThreadId) ??
null);
const providerSession =

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium state/itemSupport.ts:64

resolveV2ItemSupport returns providerSession: null when providerThread.providerSessionId is null, even when the item has a runtimeRequest whose responseCapability.providerSessionId references a session present in projection.providerSessions. This silently drops the session link for request-backed items on detached provider threads. Consider falling back to runtimeRequest.responseCapability.providerSessionId (for live capabilities) when providerThread.providerSessionId is absent.

🤖 Copy this AI Prompt to have your agent fix this:
In file @packages/client-runtime/src/state/itemSupport.ts around line 64:

`resolveV2ItemSupport` returns `providerSession: null` when `providerThread.providerSessionId` is null, even when the item has a `runtimeRequest` whose `responseCapability.providerSessionId` references a session present in `projection.providerSessions`. This silently drops the session link for request-backed items on detached provider threads. Consider falling back to `runtimeRequest.responseCapability.providerSessionId` (for `live` capabilities) when `providerThread.providerSessionId` is absent.

}),
),
),
resumeThread: (threadInput) =>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High Adapters/OpenCodeAdapterV2.ts:2386

resumeThread calls only session.get and never applies threadInput.runtimePolicy, so a resumed OpenCode session keeps the permission rules it was originally created with. Since openCodePermissionRules is applied only at session.create time, resuming a thread that was created under a looser policy leaves edit/network/command permissions enabled even after the app has switched to a stricter runtimePolicy. The next turn on the resumed thread runs with stale, overly broad permissions instead of the requested policy. Consider re-applying threadInput.runtimePolicy (or re-invoking the permission-rule setup used at create time) during resume so the session's permissions match the current policy.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/Adapters/OpenCodeAdapterV2.ts around line 2386:

`resumeThread` calls only `session.get` and never applies `threadInput.runtimePolicy`, so a resumed OpenCode session keeps the permission rules it was originally created with. Since `openCodePermissionRules` is applied only at `session.create` time, resuming a thread that was created under a looser policy leaves edit/network/command permissions enabled even after the app has switched to a stricter `runtimePolicy`. The next turn on the resumed thread runs with stale, overly broad permissions instead of the requested policy. Consider re-applying `threadInput.runtimePolicy` (or re-invoking the permission-rule setup used at create time) during resume so the session's permissions match the current policy.

);
const nativeSession = unwrapData("session.get", response);
const resumedAt = yield* DateTime.now;
const providerThread = {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High Adapters/OpenCodeAdapterV2.ts:2394

The returned providerThread keeps the stale providerInstanceId from threadInput.providerThread instead of the current adapter instance's id. Only providerSessionId is overwritten, so resuming a thread created under a different OpenCode instance yields a providerThread whose providerInstanceId no longer matches the active instance. Downstream code filters provider threads by providerInstanceId, so the resumed thread cannot be matched and later turns may create a new native thread instead of continuing the conversation. Add providerInstanceId: input.providerInstanceId to the rebuilt object.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/Adapters/OpenCodeAdapterV2.ts around line 2394:

The returned `providerThread` keeps the stale `providerInstanceId` from `threadInput.providerThread` instead of the current adapter instance's id. Only `providerSessionId` is overwritten, so resuming a thread created under a different OpenCode instance yields a `providerThread` whose `providerInstanceId` no longer matches the active instance. Downstream code filters provider threads by `providerInstanceId`, so the resumed thread cannot be matched and later turns may create a new native thread instead of continuing the conversation. Add `providerInstanceId: input.providerInstanceId` to the rebuilt object.

if (entry.status === "success" || entry.status === "cancelled") {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium Adapters/ClaudeAdapterV2.testkit.ts:476

replayMessages treats a runtime_exit entry with status: "cancelled" as a normal end-of-stream, so a replayed query that was cancelled is silently reported as successful. Tests can pass even though the recorded Claude session ended in cancellation instead of completing normally. The same file defines ClaudeReplayRuntimeExitError for the "cancelled" status, but line 476 returns before that error is ever thrown. Consider removing "cancelled" from the early-return condition so cancelled replays fail with ClaudeReplayRuntimeExitError instead of being treated as a clean completion.

-        if (entry.status === "success" || entry.status === "cancelled") {
+        if (entry.status === "success") {
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/Adapters/ClaudeAdapterV2.testkit.ts around line 476:

`replayMessages` treats a `runtime_exit` entry with `status: "cancelled"` as a normal end-of-stream, so a replayed query that was cancelled is silently reported as successful. Tests can pass even though the recorded Claude session ended in cancellation instead of completing normally. The same file defines `ClaudeReplayRuntimeExitError` for the `"cancelled"` status, but line 476 returns before that error is ever thrown. Consider removing `"cancelled"` from the early-return condition so cancelled replays fail with `ClaudeReplayRuntimeExitError` instead of being treated as a clean completion.

Comment on lines +248 to +251
const assertOutbound = (actual: CursorOutgoingFrame) => {
if (failure !== null) {
throw failure;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium Adapters/CursorAdapterV2.testkit.ts:248

When the next transcript entry is expect_outbound, waitForRun blocks on cursorAdvanced.promise (line 321). If another API call (close, listMessages, another send) hits a mismatch first, assertOutbound sets failure but never resolves cursorAdvanced, so the fiber already waiting in run.wait never wakes. A malformed or out-of-order replay leaves run.wait hanging indefinitely instead of failing fast. Consider resolving cursorAdvanced when failure is recorded so the blocked fiber can observe the failure.

   const assertOutbound = (actual: CursorOutgoingFrame) => {
     if (failure !== null) {
+      cursorAdvanced.resolve();
       throw failure;
     }
Also found in 1 other location(s)

apps/server/src/orchestration-v2/Adapters/OpenCodeAdapterV2.testkit.ts:176

OpenCodeReplayController.response can hang forever when the transcript is exhausted or the next entry is some non-matching frame that no other coroutine will consume. In that case entry at this.cursor is undefined (or remains a different non-runtime_exit entry), none of the branches advance or fail the controller, and line 176 waits on this.changed() indefinitely. A caller like sdkCall(... client.session.create/get/... ) then never resolves and the replayed provider session gets stuck instead of failing with a replay error.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/Adapters/CursorAdapterV2.testkit.ts around lines 248-251:

When the next transcript entry is `expect_outbound`, `waitForRun` blocks on `cursorAdvanced.promise` (line 321). If another API call (`close`, `listMessages`, another `send`) hits a mismatch first, `assertOutbound` sets `failure` but never resolves `cursorAdvanced`, so the fiber already waiting in `run.wait` never wakes. A malformed or out-of-order replay leaves `run.wait` hanging indefinitely instead of failing fast. Consider resolving `cursorAdvanced` when `failure` is recorded so the blocked fiber can observe the failure.

Also found in 1 other location(s):
- apps/server/src/orchestration-v2/Adapters/OpenCodeAdapterV2.testkit.ts:176 -- `OpenCodeReplayController.response` can hang forever when the transcript is exhausted or the next entry is some non-matching frame that no other coroutine will consume. In that case `entry` at `this.cursor` is `undefined` (or remains a different non-`runtime_exit` entry), none of the branches advance or fail the controller, and line 176 waits on `this.changed()` indefinitely. A caller like `sdkCall(... client.session.create/get/... )` then never resolves and the replayed provider session gets stuck instead of failing with a replay error.

if (typeof expected === "string" && typeof actual === "string") {
const expanded = expandExpectedString(expected);
if (!expanded.includes("<any>")) return expanded === actual;
const parts = expanded.split("<any>");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium scripts/acp-replay-agent.ts:92

matchesExpected treats <any> inside a string as an unanchored glob: an expected value like "prefix<any>suffix" matches "xxprefixMIDsuffixyy" instead of requiring the prefix and suffix to sit at the start and end. That lets acp-replay-agent accept mismatched outbound frames as successful replays whenever a transcript uses <any> inside a larger string. The loop searches for each segment anywhere after the previous one without anchoring the first segment to the beginning of actual or the last segment to the end. Consider anchoring the first part to actual.startsWith(parts[0]), advancing through the middle parts, and requiring actual.endsWith(parts[parts.length - 1]).

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/scripts/acp-replay-agent.ts around line 92:

`matchesExpected` treats `<any>` inside a string as an unanchored glob: an expected value like `"prefix<any>suffix"` matches `"xxprefixMIDsuffixyy"` instead of requiring the prefix and suffix to sit at the start and end. That lets `acp-replay-agent` accept mismatched outbound frames as successful replays whenever a transcript uses `<any>` inside a larger string. The loop searches for each segment anywhere after the previous one without anchoring the first segment to the beginning of `actual` or the last segment to the end. Consider anchoring the first part to `actual.startsWith(parts[0])`, advancing through the middle parts, and requiring `actual.endsWith(parts[parts.length - 1])`.

});

export function makeAcpRegistryProviderAdapterRegistryReplayLayer(transcript: AcpReplayTranscript) {
const serverConfigLayer = Layer.effect(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium Adapters/AcpRegistryAdapterV2.testkit.ts:34

makeAcpRegistryProviderAdapterRegistryReplayLayer builds its own ServerConfig via makeReplayServerConfig, which creates a separate temp directory tree from the outer replay harness. The ACP adapter resolves attachment paths via serverConfig.attachmentsDir, but uploads are persisted under the harness-level ServerConfig's attachmentsDir. In any replay scenario that includes message attachments, the adapter looks in the wrong temp directory, so attachment resolution fails with Invalid attachment id or read errors and breaks the turn. The layer should consume the existing ServerConfig from the replay harness context rather than constructing a new one.

Also found in 1 other location(s)

apps/server/src/orchestration-v2/Adapters/GrokAdapterV2.testkit.ts:27

makeGrokProviderAdapterRegistryReplayLayer also creates and provides a private ServerConfig instead of sharing the replay layer's ServerConfig. makeGrokAdapterV2 delegates to AcpAdapterV2, which resolves message attachments from serverConfig.attachmentsDir; uploaded replay attachments are stored under the outer harness config. A Grok replay scenario with attachments will therefore read from the wrong temp directory and fail the turn with attachment lookup/read errors.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/Adapters/AcpRegistryAdapterV2.testkit.ts around line 34:

`makeAcpRegistryProviderAdapterRegistryReplayLayer` builds its own `ServerConfig` via `makeReplayServerConfig`, which creates a separate temp directory tree from the outer replay harness. The ACP adapter resolves attachment paths via `serverConfig.attachmentsDir`, but uploads are persisted under the harness-level `ServerConfig`'s `attachmentsDir`. In any replay scenario that includes message attachments, the adapter looks in the wrong temp directory, so attachment resolution fails with `Invalid attachment id` or read errors and breaks the turn. The layer should consume the existing `ServerConfig` from the replay harness context rather than constructing a new one.

Also found in 1 other location(s):
- apps/server/src/orchestration-v2/Adapters/GrokAdapterV2.testkit.ts:27 -- `makeGrokProviderAdapterRegistryReplayLayer` also creates and provides a private `ServerConfig` instead of sharing the replay layer's `ServerConfig`. `makeGrokAdapterV2` delegates to `AcpAdapterV2`, which resolves message attachments from `serverConfig.attachmentsDir`; uploaded replay attachments are stored under the outer harness config. A Grok replay scenario with attachments will therefore read from the wrong temp directory and fail the turn with attachment lookup/read errors.

Comment on lines +1194 to +1199
const queryRuntime = query({
prompt: promptQueue,
options,
});
const iterator = queryRuntime[Symbol.asyncIterator]();
try {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium Adapters/ClaudeAdapterV2.testkit.ts:1194

If query() throws synchronously while opening the query (e.g., invalid options or missing Claude executable), recordClaudeStreamingQuery exits without pushing a runtime_exit error frame and without closing promptQueue. This truncates the recorded transcript immediately after query.open, silently losing the failure details that the transcript is supposed to capture.

The query() call at line 1194 sits outside the try/catch block. Move it inside the try so the error is recorded as a runtime_exit frame and the queue is closed.

+  try {
   const queryRuntime = query({
     prompt: promptQueue,
     options,
   });
   const iterator = queryRuntime[Symbol.asyncIterator]();
-  try {
     for (const [index, prompt] of input.prompts.entries()) {
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/Adapters/ClaudeAdapterV2.testkit.ts around lines 1194-1199:

If `query()` throws synchronously while opening the query (e.g., invalid options or missing Claude executable), `recordClaudeStreamingQuery` exits without pushing a `runtime_exit` error frame and without closing `promptQueue`. This truncates the recorded transcript immediately after `query.open`, silently losing the failure details that the transcript is supposed to capture.

The `query()` call at line 1194 sits outside the `try`/`catch` block. Move it inside the `try` so the error is recorded as a `runtime_exit` frame and the queue is closed.


if (entry.type === "runtime_exit") {
yield* Ref.update(state, (latest) => ({ ...latest, cursor: latest.cursor + 1 }));
if (entry.status === "success") {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium src/replay.ts:401

When a transcript contains a runtime_exit entry with status === "success", drainInbound calls Queue.end(input) and returns immediately without checking whether later expect_outbound or emit_inbound entries remain. As a result, a replay transcript with trailing unconsumed entries is accepted silently instead of raising CodexAppServerReplayIncompleteError, so replay-based tests pass even though part of the recorded interaction was never validated. Consider checking for remaining entries after the runtime_exit case and failing the replay when the cursor has not reached the end of transcript.entries.

🤖 Copy this AI Prompt to have your agent fix this:
In file @packages/effect-codex-app-server/src/replay.ts around line 401:

When a transcript contains a `runtime_exit` entry with `status === "success"`, `drainInbound` calls `Queue.end(input)` and returns immediately without checking whether later `expect_outbound` or `emit_inbound` entries remain. As a result, a replay transcript with trailing unconsumed entries is accepted silently instead of raising `CodexAppServerReplayIncompleteError`, so replay-based tests pass even though part of the recorded interaction was never validated. Consider checking for remaining entries after the `runtime_exit` case and failing the replay when the cursor has not reached the end of `transcript.entries`.

break;
case "queue_message":
messageIndex += 1;
pushDispatch(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium fixtures/shared.ts:462

In the queue_message case, dispatchMessageCommand is called without an explicit dispatchMode, so it defaults to start_immediately instead of queue_after_active. When the first replayed run has already completed by the time this step executes, the second message starts as a normal turn rather than a queued turn, silently dropping the expected queued status and queued_turn input intent. Pass dispatchMode: { type: "queue_after_active" } so the intent is preserved regardless of timing.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/testkit/fixtures/shared.ts around line 462:

In the `queue_message` case, `dispatchMessageCommand` is called without an explicit `dispatchMode`, so it defaults to `start_immediately` instead of `queue_after_active`. When the first replayed run has already completed by the time this step executes, the second message starts as a normal turn rather than a queued turn, silently dropping the expected `queued` status and `queued_turn` input intent. Pass `dispatchMode: { type: "queue_after_active" }` so the intent is preserved regardless of timing.

questions: item.questions.map((question) => ({ ...question, multiSelect: false })),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium state/threadRequests.ts:58

derivePendingThreadRequests hardcodes multiSelect: false for every question in a pending user_input_request, overwriting the question's real multiSelect value. Any request that legitimately accepts multiple answers will be presented as single-select, causing the client to render the wrong UI and preventing users from submitting the intended response. Consider preserving the original multiSelect value from item.questions instead of replacing it.

Suggested change
questions: item.questions.map((question) => ({ ...question, multiSelect: false })),
questions: item.questions.map((question) => ({ ...question })),
Also found in 1 other location(s)

apps/server/src/orchestration-v2/Adapters/GrokAdapterV2.ts:110

registerGrokAcpExtensions rebuilds each question object with only id, header, question, and options, dropping the multiSelect flag produced by extractXAiAskUserQuestions(). When Grok sends an ask_user_question request that allows multiple selections, the UI receives it as a single-select prompt and can only submit one choice, so Grok gets an incomplete answer array and the question flow behaves incorrectly.

🤖 Copy this AI Prompt to have your agent fix this:
In file @packages/client-runtime/src/state/threadRequests.ts around line 58:

`derivePendingThreadRequests` hardcodes `multiSelect: false` for every question in a pending `user_input_request`, overwriting the question's real `multiSelect` value. Any request that legitimately accepts multiple answers will be presented as single-select, causing the client to render the wrong UI and preventing users from submitting the intended response. Consider preserving the original `multiSelect` value from `item.questions` instead of replacing it.

Also found in 1 other location(s):
- apps/server/src/orchestration-v2/Adapters/GrokAdapterV2.ts:110 -- `registerGrokAcpExtensions` rebuilds each question object with only `id`, `header`, `question`, and `options`, dropping the `multiSelect` flag produced by `extractXAiAskUserQuestions()`. When Grok sends an `ask_user_question` request that allows multiple selections, the UI receives it as a single-select prompt and can only submit one choice, so Grok gets an incomplete answer array and the question flow behaves incorrectly.

assert.equal(subagent.result, "initial subagent response");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium subagent_continue/codex_output.ts:38

assertSubagentContinueOutput asserts subagent.result equals "initial subagent response", but the subagent is resumed in the second run and the projection updates subagent.result from the final wait/agent-state message, which reports "continued subagent response". The assertion will fail against the recorded transcript and block the replay contract test. Consider asserting against "continued subagent response" to match the final projected value.

Suggested change
assert.equal(subagent.result, "initial subagent response");
assert.equal(subagent.result, "continued subagent response");
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/testkit/fixtures/subagent_continue/codex_output.ts around line 38:

`assertSubagentContinueOutput` asserts `subagent.result` equals `"initial subagent response"`, but the subagent is resumed in the second run and the projection updates `subagent.result` from the final `wait`/agent-state message, which reports `"continued subagent response"`. The assertion will fail against the recorded transcript and block the replay contract test. Consider asserting against `"continued subagent response"` to match the final projected value.

Comment on lines +39 to +46
assert.deepEqual(
projection.attempts.map((attempt) => attempt.status),
["interrupted"],
);
assert.deepEqual(
projection.nodes.map((node) => node.status),
["interrupted"],
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium turn_interrupt/codex_output.ts:39

assertTurnInterruptOutput is shared by the grok and acpRegistry replay fixtures, but lines 39–45 hardcode that every attempt and node status must be "interrupted". The ACP cancel path used by Grok marks cancelled work as status: "cancelled" on the projected node/turn-item state, so the Grok and ACP fixtures will fail even when interruption handling is correct. Consider checking for either "interrupted" or "cancelled" on these collections, or scope the strict "interrupted"-only assertions to the Codex fixture.

-  assert.deepEqual(
-    projection.attempts.map((attempt) => attempt.status),
-    ["interrupted"],
-  );
-  assert.deepEqual(
-    projection.nodes.map((node) => node.status),
-    ["interrupted"],
-  );
+  assert.deepEqual(
+    projection.attempts.map((attempt) => attempt.status),
+    ["interrupted"],
+  );
+  assert.include(
+    ["interrupted", "cancelled"],
+    projection.nodes[0]?.status,
+    "ACP cancel path may mark nodes as cancelled",
+  );
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/testkit/fixtures/turn_interrupt/codex_output.ts around lines 39-46:

`assertTurnInterruptOutput` is shared by the `grok` and `acpRegistry` replay fixtures, but lines 39–45 hardcode that every attempt and node status must be `"interrupted"`. The ACP cancel path used by Grok marks cancelled work as `status: "cancelled"` on the projected node/turn-item state, so the Grok and ACP fixtures will fail even when interruption handling is correct. Consider checking for either `"interrupted"` or `"cancelled"` on these collections, or scope the strict `"interrupted"`-only assertions to the Codex fixture.

}
}

async response(operation: string): Promise<unknown> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium Adapters/OpenCodeAdapterV2.testkit.ts:151

OpenCodeReplayController.response hangs forever when the transcript is exhausted or the next entry is a non-matching frame (e.g. an emit_inbound whose frame isn't sdk.response for the requested operation, or an expect_outbound entry). In these cases entry is undefined or doesn't match any branch, so the loop falls through to this.changed() and waits indefinitely — but no other consumer will advance the cursor, so the promise never resolves. A caller like client.session.create(...) then hangs instead of failing with a replay error. Consider treating an exhausted cursor or a non-matching entry as a OpenCodeReplayMismatchError (or OpenCodeReplayIncompleteError) so the replay fails fast.

Also found in 1 other location(s)

apps/server/src/orchestration-v2/Adapters/CursorAdapterV2.testkit.ts:321

waitForRun blocks on cursorAdvanced.promise at line 321 whenever the next transcript entry is an expect_outbound. If another API call (close, listMessages, another send, etc.) hits a mismatch first, assertOutbound records failure but does not resolve cursorAdvanced, so the fiber already waiting in run.wait never wakes up to observe that failure. A malformed or out-of-order replay can therefore leave run.wait hanging indefinitely instead of failing fast.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/Adapters/OpenCodeAdapterV2.testkit.ts around line 151:

`OpenCodeReplayController.response` hangs forever when the transcript is exhausted or the next entry is a non-matching frame (e.g. an `emit_inbound` whose frame isn't `sdk.response` for the requested `operation`, or an `expect_outbound` entry). In these cases `entry` is `undefined` or doesn't match any branch, so the loop falls through to `this.changed()` and waits indefinitely — but no other consumer will advance the cursor, so the promise never resolves. A caller like `client.session.create(...)` then hangs instead of failing with a replay error. Consider treating an exhausted cursor or a non-matching entry as a `OpenCodeReplayMismatchError` (or `OpenCodeReplayIncompleteError`) so the replay fails fast.

Also found in 1 other location(s):
- apps/server/src/orchestration-v2/Adapters/CursorAdapterV2.testkit.ts:321 -- `waitForRun` blocks on `cursorAdvanced.promise` at line `321` whenever the next transcript entry is an `expect_outbound`. If another API call (`close`, `listMessages`, another `send`, etc.) hits a mismatch first, `assertOutbound` records `failure` but does not resolve `cursorAdvanced`, so the fiber already waiting in `run.wait` never wakes up to observe that failure. A malformed or out-of-order replay can therefore leave `run.wait` hanging indefinitely instead of failing fast.

Comment on lines +70 to +71
const pendingRequest =
projection.runtimeRequests.find((request) => request.status === "pending") ?? null;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium state/use-thread-selection.ts:70

threadDetailToShell selects the first pending runtime request from projection.runtimeRequests.find(...), which returns the oldest pending request rather than the newest. When multiple requests are pending, the optimistic shell exposes a stale request kind, so presentThreadShell derives the wrong hasPendingApprovals / hasPendingUserInput flags and the thread shows the incorrect pending-action state until the shell cache catches up. Consider sorting pending requests by createdAt descending and picking the latest, matching the persisted shell logic.

Suggested change
const pendingRequest =
projection.runtimeRequests.find((request) => request.status === "pending") ?? null;
const pendingRequest =
[...projection.runtimeRequests]
.filter((request) => request.status === "pending")
.sort((left, right) => right.createdAt.localeCompare(left.createdAt))[0] ?? null;
Also found in 1 other location(s)

apps/mobile/src/state/use-selected-thread-requests.ts:77

pendingRequests.approvals and pendingRequests.userInputs are used directly at lines 77-80, but derivePendingThreadRequests() preserves the insertion order of projection.runtimeRequests rather than sorting by createdAt. When a thread has multiple pending approvals or user-input requests, mobile now picks pendingRequests.*[0] from that arbitrary order, so the UI can surface and answer the wrong request while older pending items remain hidden.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/mobile/src/state/use-thread-selection.ts around lines 70-71:

`threadDetailToShell` selects the first pending runtime request from `projection.runtimeRequests.find(...)`, which returns the oldest pending request rather than the newest. When multiple requests are pending, the optimistic shell exposes a stale request kind, so `presentThreadShell` derives the wrong `hasPendingApprovals` / `hasPendingUserInput` flags and the thread shows the incorrect pending-action state until the shell cache catches up. Consider sorting pending requests by `createdAt` descending and picking the latest, matching the persisted shell logic.

Also found in 1 other location(s):
- apps/mobile/src/state/use-selected-thread-requests.ts:77 -- `pendingRequests.approvals` and `pendingRequests.userInputs` are used directly at lines `77`-`80`, but `derivePendingThreadRequests()` preserves the insertion order of `projection.runtimeRequests` rather than sorting by `createdAt`. When a thread has multiple pending approvals or user-input requests, mobile now picks `pendingRequests.*[0]` from that arbitrary order, so the UI can surface and answer the wrong request while older pending items remain hidden.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XXL 1,000+ changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants